diff mbox series

[-next,v16,19/20] riscv: detect assembler support for .option arch

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

Commit Message

Andy Chiu March 23, 2023, 2:59 p.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>
---
 arch/riscv/Kconfig | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Conor Dooley March 23, 2023, 3:26 p.m. UTC | #1
On Thu, Mar 23, 2023 at 02:59:23PM +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.
> 
> Suggested-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> ---
>  arch/riscv/Kconfig | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 36a5b6fed0d3..4f8fd4002f1d 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -244,6 +244,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)

Oh cool, I didn't expect this to work given what Nathan said in his
mail, but I gave it a whirl and it does seem to.
I suppose:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

I'd rather it be this way so that it is "hands off", as opposed to the
version check that would need updating in the future. And I guess it
means that support for V & IAS will automatically turn on for stable
kernels too once the LLVM change lands, which is nice ;)

Thanks Andy!

> +
>  source "arch/riscv/Kconfig.socs"
>  source "arch/riscv/Kconfig.errata"
>  
> @@ -442,7 +448,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
> 
>
Nathan Chancellor March 24, 2023, 2:59 p.m. UTC | #2
On Thu, Mar 23, 2023 at 03:26:14PM +0000, Conor Dooley wrote:
> On Thu, Mar 23, 2023 at 02:59:23PM +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.
> > 
> > Suggested-by: Nathan Chancellor <nathan@kernel.org>
> > Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> > ---
> >  arch/riscv/Kconfig | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 36a5b6fed0d3..4f8fd4002f1d 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -244,6 +244,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)
> 
> Oh cool, I didn't expect this to work given what Nathan said in his
> mail, but I gave it a whirl and it does seem to.

The second line is the clever part of this option that I had not
considered, as it checks for something that should error in addition to
something that shouldn't::

$ echo '.option arch, -i' | riscv64-linux-gcc -c -o /dev/null -x assembler -
{standard input}: Assembler messages:
{standard input}:1: Error: cannot + or - base extension `i' in .option arch `-i'

Looking at D123515, I see this same option test is present and appears
to error in the same manner so this should work when that change is
merged.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> I suppose:
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
> I'd rather it be this way so that it is "hands off", as opposed to the
> version check that would need updating in the future. And I guess it
> means that support for V & IAS will automatically turn on for stable
> kernels too once the LLVM change lands, which is nice ;)

Very much agreed!

> Thanks Andy!
> 
> > +
> >  source "arch/riscv/Kconfig.socs"
> >  source "arch/riscv/Kconfig.errata"
> >  
> > @@ -442,7 +448,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
> > 
> >
Andy Chiu March 28, 2023, 12:55 p.m. UTC | #3
On Fri, Mar 24, 2023 at 10:59 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Thu, Mar 23, 2023 at 03:26:14PM +0000, Conor Dooley wrote:
> > On Thu, Mar 23, 2023 at 02:59:23PM +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.
> > >
> > > Suggested-by: Nathan Chancellor <nathan@kernel.org>
> > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> > > ---
> > >  arch/riscv/Kconfig | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 36a5b6fed0d3..4f8fd4002f1d 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -244,6 +244,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)
> >
> > Oh cool, I didn't expect this to work given what Nathan said in his
> > mail, but I gave it a whirl and it does seem to.
>
> The second line is the clever part of this option that I had not
> considered, as it checks for something that should error in addition to
> something that shouldn't::
>
> $ echo '.option arch, -i' | riscv64-linux-gcc -c -o /dev/null -x assembler -
> {standard input}: Assembler messages:
> {standard input}:1: Error: cannot + or - base extension `i' in .option arch `-i'
>
> Looking at D123515, I see this same option test is present and appears
> to error in the same manner so this should work when that change is
> merged.

Thanks for checking D123515 Nathan. It was just lucky that It gets the
same test coverage in Clang. I would take a look at that aspect in the
future if the same happens.

>
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
>
> > I suppose:
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> >
> > I'd rather it be this way so that it is "hands off", as opposed to the
> > version check that would need updating in the future. And I guess it
> > means that support for V & IAS will automatically turn on for stable
> > kernels too once the LLVM change lands, which is nice ;)
>
> Very much agreed!
>
> > Thanks Andy!
> >
> > > +
> > >  source "arch/riscv/Kconfig.socs"
> > >  source "arch/riscv/Kconfig.errata"
> > >
> > > @@ -442,7 +448,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
> > >
> > >
>
>

Regards,
Andy
diff mbox series

Patch

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 36a5b6fed0d3..4f8fd4002f1d 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -244,6 +244,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"
 
@@ -442,7 +448,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"