On Thu, 6 Nov 2025, Avinash Jayakar wrote:
> On Thu, 2025-11-06 at 09:40 +0100, Richard Biener wrote:
> > On Thu, 6 Nov 2025, Avinash Jayakar wrote:
> >
> > > Hi,
> > >
> > > Based on the approach mentioned by Andrew, below is a patch to fix
> > > PR122126.
> > > Bootstrapped and regtested on powerpc64le and x86 with no
> > > regressions. Kindly
> > > review.
> > >
> > > Thanks and regards,
> > > Avinash Jayakar
> > >
> > > The function gimple_expand_vec_set_expr in the isel pass, converted
> > > VIEW_CONVERT_EXPR to VEC_SET_EXPR without checking the bounds on
> > > the index,
> > > which cause ICE on targets that supported VEC_SET_EXPR like x86 and
> > > powerpc.
> > > This patch adds a bound check on the index operand and rejects the
> > > conversion
> > > if index is out of bound.
> >
> > I think this is somewhat fragile since if we ever get sth like
> >
> > _1 = 1345677;
> > _2 = ... the access ...
> >
> > the variable bound will not be checked but TER might pull in the
> > constant
> > definition. I think targets should not ICE on UB here?
> >
>
> Thanks for the quick review. So to reproduce the scenario you mentioned
> following is a simple test case
> #define vect16 __attribute__((vector_size(16)))
> void func_54() {
> volatile vect16 unsigned BS_VAR_0;
> volatile idx = 12;
> BS_VAR_0[idx] = 4;
> }
>
> Im not so familiar with TER, is it a combination of passes like the
> constant propagation?
>
> I looked at the powerpc target to fix this issue. So there in
> rs6000_expand_vector_set function, it uses gen_vsx_set_v4si_p9 to
> generate the vec_set insn without checking the index. So during the
> next pass, since there is a constraint on the operand in define_insn
> saying const_0_to_3_operand, it fails to recognize the insn that was
> generated in the expand pass.
>
> So a check can be inserted to see the value of constant in
> rs6000_expand_vector_set function, but I am not sure what has to be
> generated since this is UB. Any suggestions here?
I'd suggest that we deal with this in the .VEC_SET internal function
expander by either expanding to a trap or to the unchanged source.
Likewise for .VEC_EXTRACT. I'll note both of these were supposed to
work on non-constant indexes - constant index operations should have
been folded to BIT_FIELD_REF/BIT_INSERT_EXPR, but of course those
foldings are only performed when they are in-bound (due to IL
checking of them).
So in the light of this it's probably reasonable to handle it like
in your patch, but then I'd simplify it by never expanding a
constant index to .VEC_SET/EXTRACT as, as said above, we should
have used other means for this.
One comment below ..
> Thanks and regards,
> Avinash Jayakar
>
> > I'm also missing a testcase here.
> >
> > Thanks,
> > Richard.
> >
> > > 2025-11-06 Avinash Jayakar <[email protected]>
> > >
> > > gcc/ChangeLog:
> > > PR tree-optimization/122126
> > > * gimple-isel.cc (gimple_expand_vec_set_extract_expr): Add
> > > bound check.
> > > ---
> > > gcc/gimple-isel.cc | 14 ++++++++++++++
> > > 1 file changed, 14 insertions(+)
> > >
> > > diff --git a/gcc/gimple-isel.cc b/gcc/gimple-isel.cc
> > > index b5dc579ff46..2593cae1aad 100644
> > > --- a/gcc/gimple-isel.cc
> > > +++ b/gcc/gimple-isel.cc
> > > @@ -102,6 +102,20 @@ gimple_expand_vec_set_extract_expr (struct
> > > function *fun,
> > > tree pos = TREE_OPERAND (ref, 1);
> > >
> > > tree view_op0 = TREE_OPERAND (op0, 0);
> > > +
> > > + tree idx = TREE_OPERAND (ref, 1);
> > > + // if index is a constant, then check the bounds
> > > + if (TREE_CODE (idx) == INTEGER_CST)
> > > + {
> > > + poly_uint64 nelts = TYPE_VECTOR_SUBPARTS (TREE_TYPE
> > > (view_op0));
> > > + // cannot analyse further if nelts is not known
> > > + if (!nelts.is_constant ())
> > > + return false;
IMO we should go forward when nelts isn't constant, in fact
> > > +
> > > + if (!tree_fits_uhwi_p (idx)
> > > + || tree_to_uhwi (idx) > nelts.to_constant ())
this should be
if (known_gt (wi::to_poly_wide (idx), nelts))
and the INTEGER_CST check be poly_int_tree_p (idx). Possibly
some conversion of nelts or idx is necessary to make them to proper
poly shape, it's always a bit tricky.
Richard.
> > > + return false;
> > > + }
> > > machine_mode outermode = TYPE_MODE (TREE_TYPE (view_op0));
> > > machine_mode extract_mode = TYPE_MODE (TREE_TYPE (ref));
> > >
> > >
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
