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

Reply via email to