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.
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.
FWIW, I think:
else if (TREE_SIZE (TREE_TYPE (val)) != INTEGER_CST)
would be a more direct way of testing for the condition.
Thanks,
Richard
