On Thu, Nov 7, 2024 at 11:13 AM Tejas Belagod <tejas.bela...@arm.com> wrote:
>
> On 11/7/24 2:36 PM, Richard Biener wrote:
> > On Thu, Nov 7, 2024 at 8:25 AM Tejas Belagod <tejas.bela...@arm.com> wrote:
> >>
> >> On 11/6/24 6:02 PM, Richard Biener wrote:
> >>> On Wed, Nov 6, 2024 at 12:49 PM Tejas Belagod <tejas.bela...@arm.com>
> >>> wrote:
> >>>>
> >>>> Ensure sizeless types don't end up trying to be canonicalised to
> >>>> BIT_FIELD_REFs.
> >>>
> >>> You mean variable-sized? But don't we know, when there's a constant
> >>> array index,
> >>> that the size is at least so this indexing is OK? So what's wrong with a
> >>> fixed position, fixed size BIT_FIELD_REF extraction of a VLA object?
> >>>
> >>> Richard.
> >>>
> >>
> >> Ah! The code and comment/description don't match, sorry. This change
> >> started out as gating out all canonicalizations of VLA vectors when I
> >> had limited understanding of how this worked, but eventually was
> >> simplified to gate in only those offsets that were known_le, but missed
> >> out fixing the comment/description. So, for eg.
> >>
> >> int foo (svint32_t v) { return v[3]; }
> >>
> >> canonicalises to a BIT_FIELD_REF <v, 32, 96>
> >>
> >> but something like:
> >>
> >> int foo (svint32_t v) { return v[4]; }
> >
> > So this is possibly out-of-bounds?
> >
> >> reduces to a VEC_EXTRACT <>
> >
> > But if out-of-bounds a VEC_EXTRACT isn't any better than a BIT_FIELD_REF,
> > no?
>
> Someone may have code protecting accesses like so:
>
> /* svcntw () returns num of 32-bit elements in a vec */
> if (svcntw () >= 8)
> return v[4];
>
> So I didn't error or warn (-Warray-bounds) for this or for that matter
> make it UB as it will be spurious. So technically, it may not be OOB access.
>
> Therefore BIT_FIELD_REFs are generated for anything within the range of
> a Adv SIMD register and anything beyond is left to be vec_extracted with
> SVE instructions.
You still didn't state the technical reason why BIT_FIELD_REF is worse than
.VEC_EXTRACT (which is introduced quite late only btw).
I'm mostly questioning that we have two different canonicalizations that oddly
depend on the constant index. I'd rather always go .VEC_EXTRACT or
always BIT_FIELD_REF (prefer that one) instead of having a mix for VLA vectors.
Richard.
>
> Thanks,
> Tejas.
>
>
> >
> >> I'll fix the comment/description.
> >>
> >> Thanks,
> >> Tejas.
> >>
> >>>> gcc/ChangeLog:
> >>>>
> >>>> * gimple-fold.cc (maybe_canonicalize_mem_ref_addr): Disallow
> >>>> sizeless
> >>>> types in BIT_FIELD_REFs.
> >>>> ---
> >>>> gcc/gimple-fold.cc | 3 ++-
> >>>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> >>>> index c19dac0dbfd..dd45d9f7348 100644
> >>>> --- a/gcc/gimple-fold.cc
> >>>> +++ b/gcc/gimple-fold.cc
> >>>> @@ -6281,6 +6281,7 @@ maybe_canonicalize_mem_ref_addr (tree *t, bool
> >>>> is_debug = false)
> >>>> && VECTOR_TYPE_P (TREE_TYPE (TREE_OPERAND (TREE_OPERAND (*t,
> >>>> 0), 0))))
> >>>> {
> >>>> tree vtype = TREE_TYPE (TREE_OPERAND (TREE_OPERAND (*t, 0), 0));
> >>>> + /* BIT_FIELD_REF can only happen on constant-size vectors. */
> >>>> if (VECTOR_TYPE_P (vtype))
> >>>> {
> >>>> tree low = array_ref_low_bound (*t);
> >>>> @@ -6294,7 +6295,7 @@ maybe_canonicalize_mem_ref_addr (tree *t, bool
> >>>> is_debug = false)
> >>>> (TYPE_SIZE (TREE_TYPE (*t))));
> >>>> widest_int ext
> >>>> = wi::add (idx, wi::to_widest (TYPE_SIZE
> >>>> (TREE_TYPE (*t))));
> >>>> - if (wi::les_p (ext, wi::to_widest (TYPE_SIZE (vtype))))
> >>>> + if (known_le (ext, wi::to_poly_widest (TYPE_SIZE
> >>>> (vtype))))
> >>>> {
> >>>> *t = build3_loc (EXPR_LOCATION (*t),
> >>>> BIT_FIELD_REF,
> >>>> TREE_TYPE (*t),
> >>>> --
> >>>> 2.25.1
> >>>>
> >>
>
