On Wed, Jul 23, 2025 at 10:05 AM Richard Sandiford <richard.sandif...@arm.com> wrote: > > Andrew Pinski <pins...@gmail.com> writes: > > On Wed, Jul 23, 2025 at 12:03 AM Richard Biener > > <richard.guent...@gmail.com> wrote: > >> > >> On Tue, Jul 22, 2025 at 7:57 PM Andrew Pinski <quic_apin...@quicinc.com> > >> wrote: > >> > > >> > The switch conversion code will generate an array with VLA vector > >> > constants in it > >> > in some cases but this does not work as the length of the vector type is > >> > not known > >> > at compile time; only the min size is known. > >> > > >> > I tried to reject this in initializer_constant_valid_p but code from the > >> > C++ front-end > >> > will call initializer_constant_valid_p for `vector_arrayx4()` and then > >> > will cause an ICE > >> > with g++.target/aarch64/sve/pr116595.C (and > >> > g++.target/riscv/rvv/autovec/pr116595.C). > >> > > >> > Built and tested for aarch64-linux-gnu. > >> > > >> > PR tree-optimization/121091 > >> > > >> > gcc/ChangeLog: > >> > > >> > * tree-switch-conversion.cc (switch_conversion::check_final_bb): > >> > Reject vector types which > >> > have a non-constant number of elements. > >> > > >> > gcc/testsuite/ChangeLog: > >> > > >> > * gcc.target/aarch64/sve/pr121091-1.c: New test. > >> > > >> > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com> > >> > --- > >> > .../gcc.target/aarch64/sve/pr121091-1.c | 25 +++++++++++++++++++ > >> > gcc/tree-switch-conversion.cc | 3 +++ > >> > 2 files changed, 28 insertions(+) > >> > create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pr121091-1.c > >> > > >> > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr121091-1.c > >> > b/gcc/testsuite/gcc.target/aarch64/sve/pr121091-1.c > >> > new file mode 100644 > >> > index 00000000000..ea2e5ce6b6a > >> > --- /dev/null > >> > +++ b/gcc/testsuite/gcc.target/aarch64/sve/pr121091-1.c > >> > @@ -0,0 +1,25 @@ > >> > +/* { dg-do compile } */ > >> > +/* { dg-options "-O2" } */ > >> > + > >> > +/* PR tree-optimization/121091 */ > >> > +/* Switch conversion would convert this > >> > + into static constant array but since > >> > + svbool_t is a VLA type, it can't be > >> > + stored in a constant pool. */ > >> > + > >> > +#include "arm_sve.h" > >> > + > >> > +svbool_t e(int mode, svbool_t pg) { > >> > + switch (mode) { > >> > + case 0: > >> > + pg = svptrue_pat_b16(SV_VL6); > >> > + break; > >> > + case 1: > >> > + pg = svpfalse_b(); > >> > + break; > >> > + case 2: > >> > + pg = svptrue_pat_b16(SV_VL2); > >> > + break; > >> > + } > >> > + return pg; > >> > +} > >> > diff --git a/gcc/tree-switch-conversion.cc > >> > b/gcc/tree-switch-conversion.cc > >> > index d0882879e61..dc9f67bd272 100644 > >> > --- a/gcc/tree-switch-conversion.cc > >> > +++ b/gcc/tree-switch-conversion.cc > >> > @@ -636,6 +636,9 @@ switch_conversion::check_final_bb () > >> > val = gimple_phi_arg_def (phi, i); > >> > if (!is_gimple_ip_invariant (val)) > >> > reason = "non-invariant value from a case"; > >> > + else if (VECTOR_TYPE_P (TREE_TYPE (val)) > >> > + && !TYPE_VECTOR_SUBPARTS (TREE_TYPE > >> > (val)).is_constant ()) > >> > + reason = "VLA vector type"; > >> > >> OK with me, but then I see ... > >> > >> > else > >> > { > >> > reloc = initializer_constant_valid_p (val, TREE_TYPE > >> > (val)); > >> > >> ... this and wonder why initializer_constant_valid_p says a VLA vector > >> constant is valid? > > > > I tried to describe why changing initializer_constant_valid_p won't > > work in the commit message: > >> > I tried to reject this in initializer_constant_valid_p but code from the > >> > C++ front-end > >> > will call initializer_constant_valid_p for `vector_arrayx4()` and then > >> > will cause an ICE > >> > with g++.target/aarch64/sve/pr116595.C (and > >> > g++.target/riscv/rvv/autovec/pr116595.C). > > > > Maybe I was not clear enough or there was not enough information there on > > it. > > I suppose VLA muddies the waters a bit. The comment above > initializer_constant_valid_p says: > > /* Return nonzero if VALUE is a valid constant-valued expression > for use in initializing a static variable; one that can be an > element of a "constant" initializer. [...] */ > > A VLA constant isn't a valid static initialiser in the sense of being > something that can be forced in its entirety to static memory. > But we do allow VLA constants to be initialised at the C/C++ (and gimple) > level to something in which only the minimum length gets nonzero values. > E.g.: > > svint32_t x = { 1, 2, 3, 4 }; > > is ok but: > > svint32_t x = { 1, 2, 3, 4, 5 }; > > isn't. Thus we can force the leading minimum length to static memory > and zero-initialise the rest.
That would also apply to the switch-conversion case and the patch is then too restrictive. > So I suppose the question is whether it's up to callers to check for > VLA types if they don't support partial initialisation, whether there > should be a new parameter to say what the caller is actually testing, > or something else. The predicate basically tests that output_constant can later handle the initializer. If that function handles the special cases above then we can let those through. > FWIW, I think: > > else if (TREE_SIZE (TREE_TYPE (val)) != INTEGER_CST) > > would be a more direct way of testing for the condition. Agreed, but as said I expected the existing initializer_constant_valid_p to reject VLA sizes. Richard. > > Thanks, > Richard