On Tue, May 27, 2025 at 2:40 PM Martin Jambor <mjam...@suse.cz> wrote:
>
> Hi,
>
> On Wed, May 21 2025, Eric Botcazou wrote:
> > Hi,
> >
> > IPA-SRA generally works fine in the presence of reverse Scalar_Storage_Order
> > by propagating the relevant flag onto the newly generated MEM_REFs. However
> > we have been recently faced with a specific Ada pattern that it doesn't
> > handle
> > correctly: 'Valid applied to a floating-point component of an aggregate type
> > with reverse Scalar_Storage_Order.
> >
> > The attribute is implemented by a call to a specific routine of the runtime
> > that expects a pointer to the object so, in the case of a component with
> > reverse SSO, the compiler first loads it from the aggregate to get back the
> > native storage order, but it does the load using an array of bytes instead
> > of
> > the floating-point type to prevent the FPU from fiddling with the value,
> > which
> > yields in the .original dump file:
> >
> > *(character[1:4] *) &F2b = VIEW_CONVERT_EXPR<character[1:4]>(item.f);
> >
> > Of course that's a bit convoluted, but it does not seem that another method
> > would be simpler or even work, and using VIEW_CONVERT_EXPR to toggle the SSO
> > is supposed to be supported in any case (unlike aliasing or type punning).
> >
> > The attached patch makes it work. While the call to storage_order_barrier_p
> > from IPA-SRA is quite natural (the regular SRA has it too), the tweak to the
> > predicate itself is needed to handle the scalar->aggregate conversion, which
> > is admittedly awkward but again without clear alternative.
> >
> > Tested on x86-64/Linux, OK for the mainline and 15 branch? Technically,
> > this
> > is a regression in GCC 10.x and later, but the pattern is so specific, even
> > in
> > Ada, that patching earlier branches does not seem worth the hassle.
> >
> >
> > 2025-05-21 Eric Botcazou <ebotca...@adacore.com>
> >
> > * ipa-sra.cc (scan_expr_access): Also disqualify storage order
> > barriers from splitting.
>
> The IPA-SRA change is OK.
>
> > * tree.h (storage_order_barrier_p): Also return false if the
> > operand of the VIEW_CONVERT_EXPR has reverse storage order.
>
> I cannot approve this one (but FWIW it looks OKish to me too).
That looks good to me.
Richard.
> Thanks,
>
> Martin
>
>
> >
> >
> > 2025-05-21 Eric Botcazou <ebotca...@adacore.com>
> >
> > * gnat.dg/sso19.adb: New test.
> > * gnat.dg/sso19_pkg.ads, gnat.dg/sso19_pkg.adb: New helper.
> >
> > --
> > Eric Botcazou
> > diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc
> > index 88bfae9502c..6e6cf895988 100644
> > --- a/gcc/ipa-sra.cc
> > +++ b/gcc/ipa-sra.cc
> > @@ -1848,6 +1848,12 @@ scan_expr_access (tree expr, gimple *stmt,
> > isra_scan_context ctx,
> > if (!desc || !desc->split_candidate)
> > return;
> >
> > + if (storage_order_barrier_p (expr))
> > + {
> > + disqualify_split_candidate (desc, "Encountered a storage order
> > barrier.");
> > + return;
> > + }
> > +
> > if (!poffset.is_constant (&offset)
> > || !psize.is_constant (&size)
> > || !pmax_size.is_constant (&max_size))
> > diff --git a/gcc/tree.h b/gcc/tree.h
> > index 99f26177628..1e41316b4c9 100644
> > --- a/gcc/tree.h
> > +++ b/gcc/tree.h
> > @@ -5499,7 +5499,7 @@ storage_order_barrier_p (const_tree t)
> > && TYPE_REVERSE_STORAGE_ORDER (TREE_TYPE (op)))
> > return true;
> >
> > - return false;
> > + return reverse_storage_order_for_component_p (op);
> > }
> >
> > /* Given a DECL or TYPE, return the scope in which it was declared, or
