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 Accepted
Commit e4bb020f3dbb83912eb6799a9d4bb79da4fd77ec
Headers show
Series riscv: Add vector ISA support | expand

Checks

Context Check Description
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be for-next at HEAD 90502d51ab90
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 6 and now 6
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 8 this patch: 8
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 8 this patch: 8
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 3 this patch: 3
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 20 lines checked
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

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

Eric Biggers Jan. 21, 2024, 1:13 a.m. UTC | #1 
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 | #2 
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 | #3 
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 | #4 
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 | #5 
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 | #6 
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"