diff mbox series

[-next,v21,23/27] riscv: detect assembler support for .option arch

Message ID 20230605110724.21391-24-andy.chiu@sifive.com (mailing list archive)
State New, archived
Headers show
Series riscv: Add vector ISA support | expand

Commit Message

Andy Chiu June 5, 2023, 11:07 a.m. UTC 
Some extensions use .option arch directive to selectively enable certain
extensions in parts of its assembly code. For example, Zbb uses it to
inform assmebler to emit bit manipulation instructions. However,
supporting of this directive only exist on GNU assembler and has not
landed on clang at the moment, making TOOLCHAIN_HAS_ZBB depend on
AS_IS_GNU.

While it is still under review at https://reviews.llvm.org/D123515, the
upcoming Vector patch also requires this feature in assembler. Thus,
provide Kconfig AS_HAS_OPTION_ARCH to detect such feature. Then
TOOLCHAIN_HAS_XXX will be turned on automatically when the feature land.

Suggested-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
Tested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
---
 arch/riscv/Kconfig | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Nathan Chancellor June 5, 2023, 3:48 p.m. UTC | #1 
Hi Andy,

On Mon, Jun 05, 2023 at 11:07:20AM +0000, Andy Chiu wrote:
> Some extensions use .option arch directive to selectively enable certain
> extensions in parts of its assembly code. For example, Zbb uses it to
> inform assmebler to emit bit manipulation instructions. However,
> supporting of this directive only exist on GNU assembler and has not
> landed on clang at the moment, making TOOLCHAIN_HAS_ZBB depend on
> AS_IS_GNU.
> 
> While it is still under review at https://reviews.llvm.org/D123515, the
> upcoming Vector patch also requires this feature in assembler. Thus,
> provide Kconfig AS_HAS_OPTION_ARCH to detect such feature. Then
> TOOLCHAIN_HAS_XXX will be turned on automatically when the feature land.

Just an FYI, this change has landed in LLVM main, so it should be in
LLVM 17 in a few months:

https://github.com/llvm/llvm-project/commit/9e8ed3403c191ab9c4903e8eeb8f732ff8a43cb4

If you have to spin another revision for some reason, consider updating
the Phabricator link to that one, as I expect that link to remain more
stable in the long run over the Phabricator one, as LLVM is planning to
eventually move away from Phabricator to GitHub pull requests. I don't
think this is worth respinning on its own (obviously, heh).

The rest of the change still looks good to me, thanks again for taking
this up.

> Suggested-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> Reviewed-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Tested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/Kconfig | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 348c0fa1fc8c..1019b519d590 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -262,6 +262,12 @@ config RISCV_DMA_NONCOHERENT
>  config AS_HAS_INSN
>  	def_bool $(as-instr,.insn r 51$(comma) 0$(comma) 0$(comma) t0$(comma) t0$(comma) zero)
>  
> +config AS_HAS_OPTION_ARCH
> +	# https://reviews.llvm.org/D123515
> +	def_bool y
> +	depends on $(as-instr, .option arch$(comma) +m)
> +	depends on !$(as-instr, .option arch$(comma) -i)
> +
>  source "arch/riscv/Kconfig.socs"
>  source "arch/riscv/Kconfig.errata"
>  
> @@ -466,7 +472,7 @@ config TOOLCHAIN_HAS_ZBB
>  	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zbb)
>  	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zbb)
>  	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
> -	depends on AS_IS_GNU
> +	depends on AS_HAS_OPTION_ARCH
>  
>  config RISCV_ISA_ZBB
>  	bool "Zbb extension support for bit manipulation instructions"
> -- 
> 2.17.1
>
Conor Dooley June 5, 2023, 4:25 p.m. UTC | #2 
On Mon, Jun 05, 2023 at 08:48:32AM -0700, Nathan Chancellor wrote:

> Just an FYI, this change has landed in LLVM main, so it should be in
> LLVM 17 in a few months:

Great! :)
Eric Biggers Jan. 21, 2024, 1:13 a.m. UTC | #3 
Hi Andy,

On Mon, Jun 05, 2023 at 11:07:20AM +0000, Andy Chiu wrote:
> +config AS_HAS_OPTION_ARCH
> +	# https://reviews.llvm.org/D123515
> +	def_bool y
> +	depends on $(as-instr, .option arch$(comma) +m)
> +	depends on !$(as-instr, .option arch$(comma) -i)

With tip-of-tree clang (llvm-project commit 85a8e5c3e0586e85), I'm seeing
AS_HAS_OPTION_ARCH be set to n.  It's the second "depends on" that makes it be
set to n, so apparently clang started accepting ".option arch -i".  What was
your intent here for checking that ".option arch -i" is not supported?  I'd
think that just the first "depends on" would be sufficient.

- Eric
Palmer Dabbelt Jan. 21, 2024, 2:55 a.m. UTC | #4 
On Sat, 20 Jan 2024 17:13:41 PST (-0800), ebiggers@kernel.org wrote:
> Hi Andy,
>
> On Mon, Jun 05, 2023 at 11:07:20AM +0000, Andy Chiu wrote:
>> +config AS_HAS_OPTION_ARCH
>> +	# https://reviews.llvm.org/D123515
>> +	def_bool y
>> +	depends on $(as-instr, .option arch$(comma) +m)
>> +	depends on !$(as-instr, .option arch$(comma) -i)
>
> With tip-of-tree clang (llvm-project commit 85a8e5c3e0586e85), I'm seeing
> AS_HAS_OPTION_ARCH be set to n.  It's the second "depends on" that makes it be
> set to n, so apparently clang started accepting ".option arch -i".  What was
> your intent here for checking that ".option arch -i" is not supported?  I'd
> think that just the first "depends on" would be sufficient.

I'm not sure what Andy's rationale was, but de3a913df6e ("RISC-V: 
Clarify the behavior of .option arch directive.") in binutils-gdb 
stopped accepting `.option arch, -i` along with fixing a handful of 
other oddities in our `.option arch` handling.

If that's all this is testing for then we should probably add some sort 
of version check for old binutils (or maybe just ignore it, looks like 
it was a bugfix and the old version was never released).

+Nelson, as he probably knows better than I do.

That said: what does LLVM do if you ask it to turn the "I" base ISA off?  
I'd argue there's no instructions left at that point...
Andy Chiu Jan. 21, 2024, 2:32 p.m. UTC | #5 
On Sun, Jan 21, 2024 at 10:55 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Sat, 20 Jan 2024 17:13:41 PST (-0800), ebiggers@kernel.org wrote:
> > Hi Andy,
> >
> > On Mon, Jun 05, 2023 at 11:07:20AM +0000, Andy Chiu wrote:
> >> +config AS_HAS_OPTION_ARCH
> >> +    # https://reviews.llvm.org/D123515
> >> +    def_bool y
> >> +    depends on $(as-instr, .option arch$(comma) +m)
> >> +    depends on !$(as-instr, .option arch$(comma) -i)
> >
> > With tip-of-tree clang (llvm-project commit 85a8e5c3e0586e85), I'm seeing
> > AS_HAS_OPTION_ARCH be set to n.  It's the second "depends on" that makes it be
> > set to n, so apparently clang started accepting ".option arch -i".  What was
> > your intent here for checking that ".option arch -i" is not supported?  I'd
> > think that just the first "depends on" would be sufficient.

The reason why I added the second check is because clang and gcc only
return an assembler warning when it does not support ".option arch" at
all. No errors were reported. So, it ended up passing the first
condition check for old toolchains that shouldn't be passing.

>
> I'm not sure what Andy's rationale was, but de3a913df6e ("RISC-V:
> Clarify the behavior of .option arch directive.") in binutils-gdb
> stopped accepting `.option arch, -i` along with fixing a handful of
> other oddities in our `.option arch` handling.
>
> If that's all this is testing for then we should probably add some sort
> of version check for old binutils (or maybe just ignore it, looks like
> it was a bugfix and the old version was never released).
>
> +Nelson, as he probably knows better than I do.
>
> That said: what does LLVM do if you ask it to turn the "I" base ISA off?
> I'd argue there's no instructions left at that point...

Maybe what we really should do is to upgrade the condition check to a
one liner shell script and grep if "Warning" is being printed. Sadly
this warning is not failing the compilation with -Werror.

I can try forming a patch on this if it feels alright to people.

Thanks,
Andy
Eric Biggers Jan. 21, 2024, 6:10 p.m. UTC | #6 
On Sun, Jan 21, 2024 at 10:32:59PM +0800, Andy Chiu wrote:
> 
> Maybe what we really should do is to upgrade the condition check to a
> one liner shell script and grep if "Warning" is being printed. Sadly
> this warning is not failing the compilation with -Werror.
> 
> I can try forming a patch on this if it feels alright to people.

What about -Wa,--fatal-warnings ?

- Eric
Nathan Chancellor Jan. 22, 2024, 10:29 p.m. UTC | #7 
On Sun, Jan 21, 2024 at 10:10:09AM -0800, Eric Biggers wrote:
> On Sun, Jan 21, 2024 at 10:32:59PM +0800, Andy Chiu wrote:
> > 
> > Maybe what we really should do is to upgrade the condition check to a
> > one liner shell script and grep if "Warning" is being printed. Sadly
> > this warning is not failing the compilation with -Werror.
> > 
> > I can try forming a patch on this if it feels alright to people.
> 
> What about -Wa,--fatal-warnings ?

I suspect that would work, the following diff appears to work for me
with a version of clang that does and does not support '.option arch',
(although I am not sure if adding -Wa,--fatal-warnings will have any
other consequences):

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index bffbd869a068..e3142ce531a0 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -315,7 +315,6 @@ config AS_HAS_OPTION_ARCH
 	# https://reviews.llvm.org/D123515
 	def_bool y
 	depends on $(as-instr, .option arch$(comma) +m)
-	depends on !$(as-instr, .option arch$(comma) -i)
 
 source "arch/riscv/Kconfig.socs"
 source "arch/riscv/Kconfig.errata"
diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
index 5a84b6443875..3ee8ecfb8c04 100644
--- a/scripts/Kconfig.include
+++ b/scripts/Kconfig.include
@@ -33,7 +33,7 @@ ld-option = $(success,$(LD) -v $(1))
 
 # $(as-instr,<instr>)
 # Return y if the assembler supports <instr>, n otherwise
-as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler-with-cpp -o /dev/null -)
+as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -Wa$(comma)--fatal-warnings -c -x assembler-with-cpp -o /dev/null -)
 
 # check if $(CC) and $(LD) exist
 $(error-if,$(failure,command -v $(CC)),C compiler '$(CC)' not found)
Eric Biggers Jan. 24, 2024, 9:58 p.m. UTC | #8 
On Mon, Jan 22, 2024 at 03:29:18PM -0700, Nathan Chancellor wrote:
> On Sun, Jan 21, 2024 at 10:10:09AM -0800, Eric Biggers wrote:
> > On Sun, Jan 21, 2024 at 10:32:59PM +0800, Andy Chiu wrote:
> > > 
> > > Maybe what we really should do is to upgrade the condition check to a
> > > one liner shell script and grep if "Warning" is being printed. Sadly
> > > this warning is not failing the compilation with -Werror.
> > > 
> > > I can try forming a patch on this if it feels alright to people.
> > 
> > What about -Wa,--fatal-warnings ?
> 
> I suspect that would work, the following diff appears to work for me
> with a version of clang that does and does not support '.option arch',
> (although I am not sure if adding -Wa,--fatal-warnings will have any
> other consequences):
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index bffbd869a068..e3142ce531a0 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -315,7 +315,6 @@ config AS_HAS_OPTION_ARCH
>  	# https://reviews.llvm.org/D123515
>  	def_bool y
>  	depends on $(as-instr, .option arch$(comma) +m)
> -	depends on !$(as-instr, .option arch$(comma) -i)
>  
>  source "arch/riscv/Kconfig.socs"
>  source "arch/riscv/Kconfig.errata"
> diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
> index 5a84b6443875..3ee8ecfb8c04 100644
> --- a/scripts/Kconfig.include
> +++ b/scripts/Kconfig.include
> @@ -33,7 +33,7 @@ ld-option = $(success,$(LD) -v $(1))
>  
>  # $(as-instr,<instr>)
>  # Return y if the assembler supports <instr>, n otherwise
> -as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler-with-cpp -o /dev/null -)
> +as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -Wa$(comma)--fatal-warnings -c -x assembler-with-cpp -o /dev/null -)
>  
>  # check if $(CC) and $(LD) exist
>  $(error-if,$(failure,command -v $(CC)),C compiler '$(CC)' not found)

It looks good to me as long as none of the as-instr users turn out to have any
expected warnings.

Maybe send the change to scripts/Kconfig.include as a separate patch?

- Eric
diff mbox series

Patch

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 348c0fa1fc8c..1019b519d590 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -262,6 +262,12 @@  config RISCV_DMA_NONCOHERENT
 config AS_HAS_INSN
 	def_bool $(as-instr,.insn r 51$(comma) 0$(comma) 0$(comma) t0$(comma) t0$(comma) zero)
 
+config AS_HAS_OPTION_ARCH
+	# https://reviews.llvm.org/D123515
+	def_bool y
+	depends on $(as-instr, .option arch$(comma) +m)
+	depends on !$(as-instr, .option arch$(comma) -i)
+
 source "arch/riscv/Kconfig.socs"
 source "arch/riscv/Kconfig.errata"
 
@@ -466,7 +472,7 @@  config TOOLCHAIN_HAS_ZBB
 	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zbb)
 	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zbb)
 	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
-	depends on AS_IS_GNU
+	depends on AS_HAS_OPTION_ARCH
 
 config RISCV_ISA_ZBB
 	bool "Zbb extension support for bit manipulation instructions"