Re: [PATCH v2] RISC-V: Introduce -mrvv-allow-misalign.
On Fri, 24 May 2024 16:50:52 PDT (-0700), jeffreya...@gmail.com wrote: On 5/24/24 5:43 PM, Palmer Dabbelt wrote: I'm only reading Zicclsm as saying both scalar and vector misaligned accesses are supported, but nothing about the performance. I think it was in the vector docs. It didn't say anything about performance, just a note that scalar & vector behavior could differ. Either way, the split naming scheme seems clearer to me. It also avoids getting mixed up by the no-scalar-misaligned, yes-vector-misaligned systems if they ever show up. So if Robin's OK with re-spinning things, let's just go that way? Works for me. Hopefully he's offline until Monday as it's rather late for him :-) So we'll pick it back up in the Tuesday meeting. Cool, no rush on my end. jeff
Re: [PATCH v2] RISC-V: Introduce -mrvv-allow-misalign.
On 5/24/24 5:43 PM, Palmer Dabbelt wrote: I'm only reading Zicclsm as saying both scalar and vector misaligned accesses are supported, but nothing about the performance. I think it was in the vector docs. It didn't say anything about performance, just a note that scalar & vector behavior could differ. Either way, the split naming scheme seems clearer to me. It also avoids getting mixed up by the no-scalar-misaligned, yes-vector-misaligned systems if they ever show up. So if Robin's OK with re-spinning things, let's just go that way? Works for me. Hopefully he's offline until Monday as it's rather late for him :-) So we'll pick it back up in the Tuesday meeting. jeff
Re: [PATCH v2] RISC-V: Introduce -mrvv-allow-misalign.
On Fri, 24 May 2024 16:41:39 PDT (-0700), jeffreya...@gmail.com wrote: On 5/24/24 5:39 PM, Palmer Dabbelt wrote: On Fri, 24 May 2024 16:31:48 PDT (-0700), jeffreya...@gmail.com wrote: On 5/24/24 11:14 AM, Palmer Dabbelt wrote: On Fri, 24 May 2024 09:19:09 PDT (-0700), Robin Dapp wrote: We should have something in doc/invoke too, this one is going to be tricky for users. We'll also have to define how this interacts with the existing -mstrict-align. Addressed the rest in the attached v2 which also fixes tests. I'm really not sure about -mstrict-align. I would have hoped that using -mstrict-align we'd never run into any movmisalign situation but that might be wishful thinking. Do we need to specify an interaction, though? For now the new options disables movmisalign so if we hit that despite -mstrict-align we'd still not vectorize it. I think we just need to write it down. I think there's two ways to encode this: either we treat scalar and vector as independent, or we couple them. If we treat them independently then we end up with four cases, it's not clear if they're all interesting. IIUC with this patch we'd be able to encode Given the ISA documents them as independent, I think we should follow suit and allow them to vary independently. I'm only reading Zicclsm as saying both scalar and vector misaligned accesses are supported, but nothing about the performance. I think it was in the vector docs. It didn't say anything about performance, just a note that scalar & vector behavior could differ. Either way, the split naming scheme seems clearer to me. It also avoids getting mixed up by the no-scalar-misaligned, yes-vector-misaligned systems if they ever show up. So if Robin's OK with re-spinning things, let's just go that way? Seems reasonable to me. Just having a regular naming scheme for the scalar/vector makes it clear what we're doing, and it's not like having the extra name for -mscalar-strict-align really costs anything. That was my thinking -- get the names right should help avoid confusion. Jeff
Re: [PATCH v2] RISC-V: Introduce -mrvv-allow-misalign.
On 5/24/24 5:39 PM, Palmer Dabbelt wrote: On Fri, 24 May 2024 16:31:48 PDT (-0700), jeffreya...@gmail.com wrote: On 5/24/24 11:14 AM, Palmer Dabbelt wrote: On Fri, 24 May 2024 09:19:09 PDT (-0700), Robin Dapp wrote: We should have something in doc/invoke too, this one is going to be tricky for users. We'll also have to define how this interacts with the existing -mstrict-align. Addressed the rest in the attached v2 which also fixes tests. I'm really not sure about -mstrict-align. I would have hoped that using -mstrict-align we'd never run into any movmisalign situation but that might be wishful thinking. Do we need to specify an interaction, though? For now the new options disables movmisalign so if we hit that despite -mstrict-align we'd still not vectorize it. I think we just need to write it down. I think there's two ways to encode this: either we treat scalar and vector as independent, or we couple them. If we treat them independently then we end up with four cases, it's not clear if they're all interesting. IIUC with this patch we'd be able to encode Given the ISA documents them as independent, I think we should follow suit and allow them to vary independently. I'm only reading Zicclsm as saying both scalar and vector misaligned accesses are supported, but nothing about the performance. I think it was in the vector docs. It didn't say anything about performance, just a note that scalar & vector behavior could differ. Seems reasonable to me. Just having a regular naming scheme for the scalar/vector makes it clear what we're doing, and it's not like having the extra name for -mscalar-strict-align really costs anything. That was my thinking -- get the names right should help avoid confusion. Jeff
Re: [PATCH v2] RISC-V: Introduce -mrvv-allow-misalign.
On Fri, 24 May 2024 16:31:48 PDT (-0700), jeffreya...@gmail.com wrote: On 5/24/24 11:14 AM, Palmer Dabbelt wrote: On Fri, 24 May 2024 09:19:09 PDT (-0700), Robin Dapp wrote: We should have something in doc/invoke too, this one is going to be tricky for users. We'll also have to define how this interacts with the existing -mstrict-align. Addressed the rest in the attached v2 which also fixes tests. I'm really not sure about -mstrict-align. I would have hoped that using -mstrict-align we'd never run into any movmisalign situation but that might be wishful thinking. Do we need to specify an interaction, though? For now the new options disables movmisalign so if we hit that despite -mstrict-align we'd still not vectorize it. I think we just need to write it down. I think there's two ways to encode this: either we treat scalar and vector as independent, or we couple them. If we treat them independently then we end up with four cases, it's not clear if they're all interesting. IIUC with this patch we'd be able to encode Given the ISA documents them as independent, I think we should follow suit and allow them to vary independently. I'm only reading Zicclsm as saying both scalar and vector misaligned accesses are supported, but nothing about the performance. * -mstrict-align: Both scalar and vector misaligned accesses are unsupported (-mrvv-allow-misalign doesn't matter). I'm not sure if there's hardware there, but given we have systems that don't support scalar misaligned accesses it seems reasonable to assume they'll also not support vector misaligned accesses. * -mno-strict-align -mno-rvv-allow-misalign: Scalar misaligned are supported, vector misaligned aren't supported. This matches our best theory of how the k230 and k1 behave, so it also seems reasonable to support. * -mno-strict-align -mrvv-allow-misalign: Both scalar and vector misaligned accesses are supported. This seems reasonable to support as it's how I'd hope big cores end up being designed, though again there's no hardware. I'd almost lean towards -m[no-]scalar-strict-align and -m[no-]vector-strict-align and deprecate -mstrict-align (aliasing it to the scalar alignment option). But I'll go with consensus here. Seems reasonable to me. Just having a regular naming scheme for the scalar/vector makes it clear what we're doing, and it's not like having the extra name for -mscalar-strict-align really costs anything. The fourth case is kind of wacky: scalar misaligned is unsupported, vector misaligned is supported. I'm not really sure why we'd end up with a system like that, but HW vendors do wacky things so it's kind of hard to predict. I've worked on one of these :-) The thinking from the designers was unaligned scalar access just wasn't that important, particularly with mem* and str* using the vector rather than scalar ops. OK then ;)
Re: [PATCH v2] RISC-V: Introduce -mrvv-allow-misalign.
On 5/24/24 11:14 AM, Palmer Dabbelt wrote: On Fri, 24 May 2024 09:19:09 PDT (-0700), Robin Dapp wrote: We should have something in doc/invoke too, this one is going to be tricky for users. We'll also have to define how this interacts with the existing -mstrict-align. Addressed the rest in the attached v2 which also fixes tests. I'm really not sure about -mstrict-align. I would have hoped that using -mstrict-align we'd never run into any movmisalign situation but that might be wishful thinking. Do we need to specify an interaction, though? For now the new options disables movmisalign so if we hit that despite -mstrict-align we'd still not vectorize it. I think we just need to write it down. I think there's two ways to encode this: either we treat scalar and vector as independent, or we couple them. If we treat them independently then we end up with four cases, it's not clear if they're all interesting. IIUC with this patch we'd be able to encode Given the ISA documents them as independent, I think we should follow suit and allow them to vary independently. * -mstrict-align: Both scalar and vector misaligned accesses are unsupported (-mrvv-allow-misalign doesn't matter). I'm not sure if there's hardware there, but given we have systems that don't support scalar misaligned accesses it seems reasonable to assume they'll also not support vector misaligned accesses. * -mno-strict-align -mno-rvv-allow-misalign: Scalar misaligned are supported, vector misaligned aren't supported. This matches our best theory of how the k230 and k1 behave, so it also seems reasonable to support. * -mno-strict-align -mrvv-allow-misalign: Both scalar and vector misaligned accesses are supported. This seems reasonable to support as it's how I'd hope big cores end up being designed, though again there's no hardware. I'd almost lean towards -m[no-]scalar-strict-align and -m[no-]vector-strict-align and deprecate -mstrict-align (aliasing it to the scalar alignment option). But I'll go with consensus here. The fourth case is kind of wacky: scalar misaligned is unsupported, vector misaligned is supported. I'm not really sure why we'd end up with a system like that, but HW vendors do wacky things so it's kind of hard to predict. I've worked on one of these :-) The thinking from the designers was unaligned scalar access just wasn't that important, particularly with mem* and str* using the vector rather than scalar ops. jeff
Re: [PATCH v2] RISC-V: Introduce -mrvv-allow-misalign.
> * -mstrict-align: Both scalar and vector misaligned accesses are > unsupported (-mrvv-allow-misalign doesn't matter). I'm not sure if > there's hardware there, but given we have systems that don't support > scalar misaligned accesses it seems reasonable to assume they'll also > not support vector misaligned accesses. As a data point, and contrary to what I said/hoped before: There are examples where -mstrict-align and -mrvv-allow-misalign vectorizes code and produces unaligned vector accesses. I haven't looked into that area of the vectorizer for a while but it doesn't appear as if we regard STRICT_ALIGNMENT there at all. We keep track of the known misalignments (via peeling etc.) and either handle them via movmisalign or give up. Same for unknown misalignment but all unaffected by -mstrict-align. We could have -mrvv-allow-misalign have an "| STRICT_ALIGNMENT" to get to the behavior you described but right now it's not like that. And AFAICT -mstrict-align behaves the same way for other targets, regardless if they support unaligned vector accesses or not. So, right now, I'd tend towards describing that both flags are independent and affect either only scalar or only vector code. Maybe we should rename the whole thing to -mrvv-strict-align? Might make it even more confusing, though. Regards Robin
Re: [PATCH v2] RISC-V: Introduce -mrvv-allow-misalign.
On Fri, 24 May 2024 09:19:09 PDT (-0700), Robin Dapp wrote:
We should have something in doc/invoke too, this one is going to be
tricky for users. We'll also have to define how this interacts with
the existing -mstrict-align.
Addressed the rest in the attached v2 which also fixes tests.
I'm really not sure about -mstrict-align. I would have hoped that using
-mstrict-align we'd never run into any movmisalign situation but that
might be wishful thinking. Do we need to specify an
interaction, though? For now the new options disables movmisalign so
if we hit that despite -mstrict-align we'd still not vectorize it.
I think we just need to write it down. I think there's two ways to
encode this: either we treat scalar and vector as independent, or we
couple them. If we treat them independently then we end up with four
cases, it's not clear if they're all interesting. IIUC with this patch
we'd be able to encode
* -mstrict-align: Both scalar and vector misaligned accesses are
unsupported (-mrvv-allow-misalign doesn't matter). I'm not sure if
there's hardware there, but given we have systems that don't support
scalar misaligned accesses it seems reasonable to assume they'll also
not support vector misaligned accesses.
* -mno-strict-align -mno-rvv-allow-misalign: Scalar misaligned are
supported, vector misaligned aren't supported. This matches our best
theory of how the k230 and k1 behave, so it also seems reasonable to
support.
* -mno-strict-align -mrvv-allow-misalign: Both scalar and vector
misaligned accesses are supported. This seems reasonable to support
as it's how I'd hope big cores end up being designed, though again
there's no hardware.
The fourth case is kind of wacky: scalar misaligned is unsupported,
vector misaligned is supported. I'm not really sure why we'd end up
with a system like that, but HW vendors do wacky things so it's kind of
hard to predict.
IMO it's fine if we're defining that as an unencodeable case it's fine,
we can always add something later. We should just write it down so
nobody's confused.
Regtested on rv64gcv_zvfh_zvbb.
Regards
Robin
This patch changes the default from always enabling movmisalign to
not enabling it. It adds an option to override the default and adds
generic-ooo to the uarchs that support misaligned vector access.
It also adds a check_effective_target_riscv_v_misalign_ok to the
testsuite which enables or disables the vector misalignment tests
depending on whether the target under test can execute a misaligned
vle32.
gcc/ChangeLog:
* config/riscv/riscv-opts.h (TARGET_VECTOR_MISALIGN_SUPPORTED):
Move from here...
* config/riscv/riscv.h (TARGET_VECTOR_MISALIGN_SUPPORTED):
...to here and make dependent on uarch and rvv_allow_misalign.
* config/riscv/riscv.opt: Add -mrvv-allow-unaligned.
gcc/testsuite/ChangeLog:
* lib/target-supports.exp: Add
check_effective_target_riscv_v_misalign_ok.
* gcc.dg/vect/costmodel/riscv/rvv/dynamic-lmul2-7.c: Add
-mrvv-allow-misalign.
* gcc.dg/vect/costmodel/riscv/rvv/vla_vs_vls-10.c: Ditto.
* gcc.dg/vect/costmodel/riscv/rvv/vla_vs_vls-11.c: Ditto.
* gcc.dg/vect/costmodel/riscv/rvv/vla_vs_vls-12.c: Ditto.
* gcc.dg/vect/costmodel/riscv/rvv/vla_vs_vls-8.c: Ditto.
* gcc.dg/vect/costmodel/riscv/rvv/vla_vs_vls-9.c: Ditto.
* gcc.target/riscv/rvv/autovec/vls/misalign-1.c:
---
gcc/config/riscv/riscv-opts.h | 3 --
gcc/config/riscv/riscv.cc | 18 ++
gcc/config/riscv/riscv.h | 6
gcc/config/riscv/riscv.opt| 5 +++
gcc/doc/invoke.texi | 5 +++
.../costmodel/riscv/rvv/dynamic-lmul2-7.c | 2 +-
.../vect/costmodel/riscv/rvv/vla_vs_vls-10.c | 2 +-
.../vect/costmodel/riscv/rvv/vla_vs_vls-11.c | 2 +-
.../vect/costmodel/riscv/rvv/vla_vs_vls-12.c | 2 +-
.../vect/costmodel/riscv/rvv/vla_vs_vls-8.c | 2 +-
.../vect/costmodel/riscv/rvv/vla_vs_vls-9.c | 2 +-
.../riscv/rvv/autovec/vls/misalign-1.c| 2 +-
gcc/testsuite/lib/target-supports.exp | 34 +--
13 files changed, 73 insertions(+), 12 deletions(-)
diff --git a/gcc/config/riscv/riscv-opts.h b/gcc/config/riscv/riscv-opts.h
index 1b2dd5757a8..f58a07abffc 100644
--- a/gcc/config/riscv/riscv-opts.h
+++ b/gcc/config/riscv/riscv-opts.h
@@ -147,9 +147,6 @@ enum rvv_vector_bits_enum {
? 0
\
: 32 << (__builtin_popcount (opts->x_riscv_zvl_flags) - 1))
-/* TODO: Enable RVV movmisalign by default for now. */
-#define TARGET_VECTOR_MISALIGN_SUPPORTED 1
-
/* The maximmum LMUL according to user configuration. */
#define TARGET_MAX_LMUL
\
(int) (rvv_max_lmul == RVV_DYNAMIC ? RVV_M8 : rvv_max_lmul)
diff --git
