On Wed, Jul 23, 2025 at 1:37 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> 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.
I am going to take another stab at changing
initializer_constant_valid_p . If I cannot get it to work in a week, I
submit the TREE_SIZE() != INTEGER_CST patch.
Thanks,
Andrew
>
> Richard.
>
> >
> > Thanks,
> > Richard
