diff mbox series

[-next,v13,19/19] riscv: Enable Vector code to be built

Message ID 20230125142056.18356-20-andy.chiu@sifive.com (mailing list archive)
State Superseded
Delegated to: Palmer Dabbelt
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
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 13 and now 13
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 2051 this patch: 0
conchuod/alphanumeric_selects success Out of order selects before the patch: 57 and now 57
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 2 this patch: 2
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 29 lines checked
conchuod/source_inline success Was 0 now: 0
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 Jan. 25, 2023, 2:20 p.m. UTC
From: Guo Ren <guoren@linux.alibaba.com>

This patch adds a config which enables vector feature from the kernel
space.

Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
Suggested-by: Vineet Gupta <vineetg@rivosinc.com>
Suggested-by: Atish Patra <atishp@atishpatra.org>
Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
---
 arch/riscv/Kconfig  | 10 ++++++++++
 arch/riscv/Makefile |  7 +++++++
 2 files changed, 17 insertions(+)

Comments

Conor Dooley Jan. 25, 2023, 9:04 p.m. UTC | #1
Hey Andy,

Thanks for respinning this, I think a lot of people will be happy to see
it!

On Wed, Jan 25, 2023 at 02:20:56PM +0000, Andy Chiu wrote:

> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 12d91b0a73d8..67411cdc836f 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -52,6 +52,13 @@ riscv-march-$(CONFIG_ARCH_RV32I)	:= rv32ima
>  riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64ima
>  riscv-march-$(CONFIG_FPU)		:= $(riscv-march-y)fd
>  riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
> +riscv-march-$(CONFIG_RISCV_ISA_V)	:= $(riscv-march-y)v
> +
> +ifeq ($(CONFIG_RISCV_ISA_V), y)
> +ifeq ($(CONFIG_CC_IS_CLANG), y)
> +        riscv-march-y += -mno-implicit-float -menable-experimental-extensions
> +endif
> +endif

Uh, so I don't think this was actually tested with (a recent version of)
clang:
clang-15: error: unknown argument: '-menable-experimental-extensions_zicbom_zihintpause'

Firstly, no-implicit-float is a CFLAG, so why add it to march?
There is an existing patch on the list for enabling this flag, but I
recall Palmer saying that it was not actually needed?
Palmer, do you remember why that was?

I dunno what enable-experimental-extensions is, but I can guess. Do we
really want to enable vector for toolchains where the support is
considered experimental? I'm not au fait with the details of clang
versions nor versions of the Vector spec, so take the following with a
bit of a pinch of salt...
Since you've allowed this to be built with anything later than clang 13,
does that mean that different versions of clang may generate vector code
that are not compatible?
I'm especially concerned by:
https://github.com/riscv/riscv-v-spec/releases/tag/0.9
which appears to be most recently released version of the spec, prior to
clang/llvm 13 being released.

> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index e2b656043abf..f4299ba9a843 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -416,6 +416,16 @@ config RISCV_ISA_SVPBMT
>  
>  	   If you don't know what to do here, say Y.
>  
> +config RISCV_ISA_V
> +	bool "VECTOR extension support"
> +	depends on GCC_VERSION >= 120000 || CLANG_VERSION >= 130000

Are these definitely the versions you want to support?
What are the earliest (upstream) versions that support the frozen
version of the vector spec?

Also, please copy what has been done with "TOOLCHAIN_HAS_FOO" for other
extensions and check this support with cc-option instead. Similarly,
you'll need to gate this support on the linker being capable of
accepting vector:
/stuff/toolchains/gcc-11/bin/riscv64-unknown-linux-gnu-ld: -march=rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0_v1p0_zihintpause2p0_zve32f1p0_zve32x1p0_zve64d1p0_zve64f1p0_zve64x1p0_zvl128b1p0_zvl32b1p0_zvl64b1p0: prefixed ISA extension must separate with _
/stuff/toolchains/gcc-11/bin/riscv64-unknown-linux-gnu-ld: failed to merge target specific data of file arch/riscv/kernel/vdso/vgettimeofday.o

> +	default n

I forget, but is the reason for this being default n, when the others
are default y a conscious choice?
I'm a bit of a goldfish sometimes memory wise, and I don't remember if
that was an outcome of the previous discussions.
If it is intentionally different, that needs to be in the changelog IMO.

> +	help
> +	  Say N here if you want to disable all vector related procedure
> +	  in the kernel.
> +
> +	  If you don't know what to do here, say Y.
> +
>  config TOOLCHAIN_HAS_ZICBOM

^ you can use this one here as an example :)

I'll reply here again once the patchwork automation has given the series
a once over and see if it comes up with any other build issues.
Thanks,
Conor.
Jessica Clarke Jan. 25, 2023, 9:38 p.m. UTC | #2
On 25 Jan 2023, at 21:04, Conor Dooley <conor@kernel.org> wrote:
> 
> Hey Andy,
> 
> Thanks for respinning this, I think a lot of people will be happy to see
> it!
> 
> On Wed, Jan 25, 2023 at 02:20:56PM +0000, Andy Chiu wrote:
> 
>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
>> index 12d91b0a73d8..67411cdc836f 100644
>> --- a/arch/riscv/Makefile
>> +++ b/arch/riscv/Makefile
>> @@ -52,6 +52,13 @@ riscv-march-$(CONFIG_ARCH_RV32I)	:= rv32ima
>> riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64ima
>> riscv-march-$(CONFIG_FPU)		:= $(riscv-march-y)fd
>> riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
>> +riscv-march-$(CONFIG_RISCV_ISA_V)	:= $(riscv-march-y)v
>> +
>> +ifeq ($(CONFIG_RISCV_ISA_V), y)
>> +ifeq ($(CONFIG_CC_IS_CLANG), y)
>> +        riscv-march-y += -mno-implicit-float -menable-experimental-extensions
>> +endif
>> +endif
> 
> Uh, so I don't think this was actually tested with (a recent version of)
> clang:
> clang-15: error: unknown argument: '-menable-experimental-extensions_zicbom_zihintpause'
> 
> Firstly, no-implicit-float is a CFLAG, so why add it to march?
> There is an existing patch on the list for enabling this flag, but I
> recall Palmer saying that it was not actually needed?
> Palmer, do you remember why that was?
> 
> I dunno what enable-experimental-extensions is, but I can guess. Do we
> really want to enable vector for toolchains where the support is
> considered experimental? I'm not au fait with the details of clang
> versions nor versions of the Vector spec, so take the following with a
> bit of a pinch of salt...
> Since you've allowed this to be built with anything later than clang 13,
> does that mean that different versions of clang may generate vector code
> that are not compatible?
> I'm especially concerned by:
> https://github.com/riscv/riscv-v-spec/releases/tag/0.9
> which appears to be most recently released version of the spec, prior to
> clang/llvm 13 being released.

For implementations of unratified extensions you both have to enable
them with -menable-experimental-extensions and have to explicitly
specify the version in the -march string specifically so this isn’t a
concern. Only once ratified can you use the unversioned extension,
which is implicitly the ratified version (ignoring the whole i2p0 vs
i2p1 fiasco).

But no, you probably don’t want experimental implementations, which can
exist when the ratified version is implemented in theory (so there’s no
compatibility concern based on ISA changes) but isn’t deemed
production-ready (e.g. potential ABI instability in the case of
something like V).

Jess
Conor Dooley Jan. 25, 2023, 10:24 p.m. UTC | #3
On Wed, Jan 25, 2023 at 09:38:00PM +0000, Jessica Clarke wrote:
> On 25 Jan 2023, at 21:04, Conor Dooley <conor@kernel.org> wrote:
> > On Wed, Jan 25, 2023 at 02:20:56PM +0000, Andy Chiu wrote:
> > 
> >> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> >> index 12d91b0a73d8..67411cdc836f 100644
> >> --- a/arch/riscv/Makefile
> >> +++ b/arch/riscv/Makefile
> >> @@ -52,6 +52,13 @@ riscv-march-$(CONFIG_ARCH_RV32I)	:= rv32ima
> >> riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64ima
> >> riscv-march-$(CONFIG_FPU)		:= $(riscv-march-y)fd
> >> riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
> >> +riscv-march-$(CONFIG_RISCV_ISA_V)	:= $(riscv-march-y)v
> >> +
> >> +ifeq ($(CONFIG_RISCV_ISA_V), y)
> >> +ifeq ($(CONFIG_CC_IS_CLANG), y)
> >> +        riscv-march-y += -mno-implicit-float -menable-experimental-extensions
> >> +endif
> >> +endif
> > 
> > Uh, so I don't think this was actually tested with (a recent version of)
> > clang:
> > clang-15: error: unknown argument: '-menable-experimental-extensions_zicbom_zihintpause'
> > 
> > Firstly, no-implicit-float is a CFLAG, so why add it to march?
> > There is an existing patch on the list for enabling this flag, but I
> > recall Palmer saying that it was not actually needed?
> > Palmer, do you remember why that was?
> > 
> > I dunno what enable-experimental-extensions is, but I can guess. Do we
> > really want to enable vector for toolchains where the support is
> > considered experimental? I'm not au fait with the details of clang
> > versions nor versions of the Vector spec, so take the following with a
> > bit of a pinch of salt...
> > Since you've allowed this to be built with anything later than clang 13,
> > does that mean that different versions of clang may generate vector code
> > that are not compatible?
> > I'm especially concerned by:
> > https://github.com/riscv/riscv-v-spec/releases/tag/0.9
> > which appears to be most recently released version of the spec, prior to
> > clang/llvm 13 being released.
> 
> For implementations of unratified extensions you both have to enable
> them with -menable-experimental-extensions and have to explicitly
> specify the version in the -march string specifically so this isn’t a
> concern. Only once ratified can you use the unversioned extension,
> which is implicitly the ratified version (ignoring the whole i2p0 vs
> i2p1 fiasco).

Ahh, thanks for the clarification Jess.

> But no, you probably don’t want experimental implementations, which can
> exist when the ratified version is implemented in theory (so there’s no
> compatibility concern based on ISA changes) but isn’t deemed
> production-ready (e.g. potential ABI instability in the case of
> something like V).

And I guess, if you turn it on for one, it's on for all.
While the vector extension might be okay in that regard, another
extension well not be okay to use the "unversioned" experimental version
of. Sounds like removing that option and picking the version of clang
that adds the actual implementation is a better approach, at least IMO.
Andy Chiu Jan. 30, 2023, 6:38 a.m. UTC | #4
On Thu, Jan 26, 2023 at 5:04 AM Conor Dooley <conor@kernel.org> wrote:
> Uh, so I don't think this was actually tested with (a recent version of)
> clang:
> clang-15: error: unknown argument: '-menable-experimental-extensions_zicbom_zihintpause'
>
> Firstly, no-implicit-float is a CFLAG, so why add it to march?
> There is an existing patch on the list for enabling this flag, but I
> recall Palmer saying that it was not actually needed?
> Palmer, do you remember why that was?
>
> I dunno what enable-experimental-extensions is, but I can guess. Do we
> really want to enable vector for toolchains where the support is
> considered experimental? I'm not au fait with the details of clang
> versions nor versions of the Vector spec, so take the following with a
> bit of a pinch of salt...
> Since you've allowed this to be built with anything later than clang 13,
> does that mean that different versions of clang may generate vector code
> that are not compatible?
Thanks for pointing this out. We found that Vector in clang13 was
still an experimental feature. And the first version of clang which
lists Vector v1.0 (ratified) as a standard support is clang14. Thus,
we will require the minimum clang toolchain version to be 14 in the
following revision.
> I'm especially concerned by:
> https://github.com/riscv/riscv-v-spec/releases/tag/0.9
> which appears to be most recently released version of the spec, prior to
> clang/llvm 13 being released.
>
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index e2b656043abf..f4299ba9a843 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -416,6 +416,16 @@ config RISCV_ISA_SVPBMT
> >
> >          If you don't know what to do here, say Y.
> >
> > +config RISCV_ISA_V
> > +     bool "VECTOR extension support"
> > +     depends on GCC_VERSION >= 120000 || CLANG_VERSION >= 130000
>
> Are these definitely the versions you want to support?
> What are the earliest (upstream) versions that support the frozen
> version of the vector spec?
It is 14 for clang and 2.38 for GNU binutils
>
> Also, please copy what has been done with "TOOLCHAIN_HAS_FOO" for other
> extensions and check this support with cc-option instead. Similarly,
Yes, updating it.
> you'll need to gate this support on the linker being capable of
> accepting vector:
> /stuff/toolchains/gcc-11/bin/riscv64-unknown-linux-gnu-ld: -march=rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0_v1p0_zihintpause2p0_zve32f1p0_zve32x1p0_zve64d1p0_zve64f1p0_zve64x1p0_zvl128b1p0_zvl32b1p0_zvl64b1p0: prefixed ISA extension must separate with _
> /stuff/toolchains/gcc-11/bin/riscv64-unknown-linux-gnu-ld: failed to merge target specific data of file arch/riscv/kernel/vdso/vgettimeofday.o
>
> > +     default n
>
> I forget, but is the reason for this being default n, when the others
> are default y a conscious choice?
Yes, I think it could be y if V is allocated in the first-use trap, as
far as I'm concerned. Hey Vineet, do you have any comments about that?
> I'm a bit of a goldfish sometimes memory wise, and I don't remember if
> that was an outcome of the previous discussions.
> If it is intentionally different, that needs to be in the changelog IMO.
>
> > +     help
> > +       Say N here if you want to disable all vector related procedure
> > +       in the kernel.
> > +
> > +       If you don't know what to do here, say Y.
> > +
> >  config TOOLCHAIN_HAS_ZICBOM
>
> ^ you can use this one here as an example :)
Ok! Thanks
>
> I'll reply here again once the patchwork automation has given the series
> a once over and see if it comes up with any other build issues.
> Thanks,
> Conor.
>
Thanks,
Andy
Andy Chiu Jan. 30, 2023, 7:46 a.m. UTC | #5
On Thu, Jan 26, 2023 at 5:04 AM Conor Dooley <conor@kernel.org> wrote:
> Firstly, no-implicit-float is a CFLAG, so why add it to march?
I placed it in march because I thought we need the flag in vdso. And,
KBUILD_CFLAGS is not enough for vdso. However, I think we don't need
this flag in vdso since it is run in user space anyway.
> There is an existing patch on the list for enabling this flag, but I
> recall Palmer saying that it was not actually needed?
The flag is needed for clang builds to prevent auto-vectorization from
using V in the kernel code [1].

> Palmer, do you remember why that was?
The discussion[2] suggested that we need this flag, IIUC. But somehow
the patch did make it into the tree.
>
[1]https://lore.kernel.org/all/CAOnJCULtT-y9vo6YhW7bW9XyKRdod-hvFfr02jHVamR_LcsKdA@mail.gmail.com/
[2]https://lore.kernel.org/all/20221216185012.2342675-1-abdulras@google.com/
Conor Dooley Jan. 30, 2023, 8:13 a.m. UTC | #6
On Mon, Jan 30, 2023 at 03:46:32PM +0800, Andy Chiu wrote:
> On Thu, Jan 26, 2023 at 5:04 AM Conor Dooley <conor@kernel.org> wrote:
> > Firstly, no-implicit-float is a CFLAG, so why add it to march?
> I placed it in march because I thought we need the flag in vdso. And,
> KBUILD_CFLAGS is not enough for vdso. However, I think we don't need
> this flag in vdso since it is run in user space anyway.
> > There is an existing patch on the list for enabling this flag, but I
> > recall Palmer saying that it was not actually needed?
> The flag is needed for clang builds to prevent auto-vectorization from
> using V in the kernel code [1].
> 
> > Palmer, do you remember why that was?
> The discussion[2] suggested that we need this flag, IIUC. But somehow
> the patch did make it into the tree.

I know, in [1] I left an R-b as the patch seemed reasonable to me.
Palmer mentioned some reason for not thinking it was actually needed but
not on-list, so I was hoping he'd comment!

And I suppose, it never got any further attention as it isn't needed by
any in-tree code?

> [1]https://lore.kernel.org/all/CAOnJCULtT-y9vo6YhW7bW9XyKRdod-hvFfr02jHVamR_LcsKdA@mail.gmail.com/
> [2]https://lore.kernel.org/all/20221216185012.2342675-1-abdulras@google.com/
Vineet Gupta Jan. 30, 2023, 6:38 p.m. UTC | #7
On 1/29/23 22:38, Andy Chiu wrote:
>>> +     default n
>> I forget, but is the reason for this being default n, when the others
>> are default y a conscious choice?
> Yes, I think it could be y if V is allocated in the first-use trap, as
> far as I'm concerned. Hey Vineet, do you have any comments about that?

Yes I think this can be enabled by default now that everything is 
allocated on demand.
FWIW thread_struct would have 5 word overhead due to struct 
__riscv_v_state but nothing I would worry about too much.

-Vineet
Conor Dooley Feb. 8, 2023, 6:19 p.m. UTC | #8
Hey Andy,

On Mon, Jan 30, 2023 at 08:13:20AM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 03:46:32PM +0800, Andy Chiu wrote:
> > On Thu, Jan 26, 2023 at 5:04 AM Conor Dooley <conor@kernel.org> wrote:
> > > Firstly, no-implicit-float is a CFLAG, so why add it to march?
> > I placed it in march because I thought we need the flag in vdso. And,
> > KBUILD_CFLAGS is not enough for vdso. However, I think we don't need
> > this flag in vdso since it is run in user space anyway.
> > > There is an existing patch on the list for enabling this flag, but I
> > > recall Palmer saying that it was not actually needed?
> > The flag is needed for clang builds to prevent auto-vectorization from
> > using V in the kernel code [1].
> > 
> > > Palmer, do you remember why that was?
> > The discussion[2] suggested that we need this flag, IIUC. But somehow
> > the patch did make it into the tree.
> 
> I know, in [1] I left an R-b as the patch seemed reasonable to me.
> Palmer mentioned some reason for not thinking it was actually needed but
> not on-list, so I was hoping he'd comment!

Palmer replied there today with his rationale & an expectation that we
do the same thing for vector as we did for float:
https://lore.kernel.org/linux-riscv/mhng-4c71ada6-003c-414f-9a74-efa3ccd2856b@palmer-ri-x1c9/T/#m366779709bbcf7672b5277b3bb27a7d6ce6c6115

> 
> And I suppose, it never got any further attention as it isn't needed by
> any in-tree code?
> 
> > [1]https://lore.kernel.org/all/CAOnJCULtT-y9vo6YhW7bW9XyKRdod-hvFfr02jHVamR_LcsKdA@mail.gmail.com/
> > [2]https://lore.kernel.org/all/20221216185012.2342675-1-abdulras@google.com/

Cheers,
Conor.
diff mbox series

Patch

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index e2b656043abf..f4299ba9a843 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -416,6 +416,16 @@  config RISCV_ISA_SVPBMT
 
 	   If you don't know what to do here, say Y.
 
+config RISCV_ISA_V
+	bool "VECTOR extension support"
+	depends on GCC_VERSION >= 120000 || CLANG_VERSION >= 130000
+	default n
+	help
+	  Say N here if you want to disable all vector related procedure
+	  in the kernel.
+
+	  If you don't know what to do here, say Y.
+
 config TOOLCHAIN_HAS_ZICBOM
 	bool
 	default y
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 12d91b0a73d8..67411cdc836f 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -52,6 +52,13 @@  riscv-march-$(CONFIG_ARCH_RV32I)	:= rv32ima
 riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64ima
 riscv-march-$(CONFIG_FPU)		:= $(riscv-march-y)fd
 riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
+riscv-march-$(CONFIG_RISCV_ISA_V)	:= $(riscv-march-y)v
+
+ifeq ($(CONFIG_RISCV_ISA_V), y)
+ifeq ($(CONFIG_CC_IS_CLANG), y)
+        riscv-march-y += -mno-implicit-float -menable-experimental-extensions
+endif
+endif
 
 # Newer binutils versions default to ISA spec version 20191213 which moves some
 # instructions from the I extension to the Zicsr and Zifencei extensions.