[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 --- Comment #24 from GCC Commits --- The releases/gcc-14 branch has been updated by Richard Biener : https://gcc.gnu.org/g:0b7ec50ae2959153650c0b3dc134c8872ff9fcfc commit r14-10456-g0b7ec50ae2959153650c0b3dc134c8872ff9fcfc Author: Jan Hubicka Date: Thu May 16 15:33:55 2024 +0200 Fix points_to_local_or_readonly_memory_p wrt TARGET_MEM_REF TARGET_MEM_REF can be used to offset constant base into a memory object (to produce lea instruction). This confuses points_to_local_or_readonly_memory_p which treats the constant address as a base of the access. Bootstrapped/regtsted x86_64-linux, comitted. Honza gcc/ChangeLog: PR ipa/113787 * ipa-fnsummary.cc (points_to_local_or_readonly_memory_p): Do not look into TARGET_MEM_REFS with constant opreand 0. gcc/testsuite/ChangeLog: * gcc.c-torture/execute/pr113787.c: New test. (cherry picked from commit 96d53252aefcbc2fe419c4c3b4bcd3fc03d4d187)
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 Richard Biener changed: What|Removed |Added Target Milestone|12.4|12.5 --- Comment #23 from Richard Biener --- GCC 12.4 is being released, retargeting bugs to GCC 12.5.
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 Jan Hubicka changed: What|Removed |Added Summary|[12/13/14/15 Regression]|[12/13/14 Regression] Wrong |Wrong code at -O with |code at -O with ipa-modref |ipa-modref on aarch64 |on aarch64 --- Comment #22 from Jan Hubicka --- Fixed on trunk so far
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 --- Comment #19 from Jan Hubicka --- > Note I didn't check if it helps the testcase .. I will check. > > > > > > > A "nicer" solution might be to add a informational operand > > > to TARGET_MEM_REF, representing the base pointer to be used for > > > alias/points-to purposes. But if that's not invariant it might > > > keep some otherwise unnecessary definition stmts live. > > > > Yep, I see that forcing extra IV to track original semantics would be > > trouble here. I think that after iv-opts we should be done with more > > fancy propagation across loops. > > > > However, to avoid ipa-modref summary degradation, perhaps scheduling the > > pass before ivopts would make sense... > > We also have that other bug where store-merging breaks the IPA summary > which gets prevailed in the late modref, so moving it around doesn't > solve all of the IL related issues ... I did not mean to paper around the fact that we produce wrong code with TARGET_MEM_REFs (we ought to fix that). If ivopts makes it difficult to track bases of memory accesses, we may get better summaries if we built them earlier. The purpose for late mod-ref is to analyze the function body as cleaned up as possible (so we do not get confused i.e. by dead memory accesses and other stuff we see before inlining) but we do not want to lower the representation too much, since that is generally lossy too. I will look at the store merging issue. Honza
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787
--- Comment #18 from rguenther at suse dot de ---
On Wed, 14 Feb 2024, hubicka at ucw dot cz wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787
>
> --- Comment #17 from Jan Hubicka ---
> > > I guess PTA gets around by tracking points-to set also for non-pointer
> > > types and consequently it also gives up on any such addition.
> >
> > It does. But note it does _not_ for POINTER_PLUS where it treats
> > the offset operand as non-pointer.
> >
> > > I think it is ipa-prop.c::unadjusted_ptr_and_unit_offset. It accepts
> > > pointer_plus expression, but does not look through POINTER_PLUS.
> > > We can restrict it further, but tracking base pointer is quite useful,
> > > so it would be nice to not give up completely.
> >
> > It looks like that function might treat that
> >
> > ADDR_EXPR >
> >
> > as integer_zerop base. It does
> >
> > if (TREE_CODE (op) == ADDR_EXPR)
> > {
> > poly_int64 extra_offset = 0;
> > tree base = get_addr_base_and_unit_offset (TREE_OPERAND (op, 0),
> > );
> > if (!base)
> > {
> > base = get_base_address (TREE_OPERAND (op, 0));
> > if (TREE_CODE (base) != MEM_REF)
> > break;
> > offset_known = false;
> > }
> > else
> > {
> > if (TREE_CODE (base) != MEM_REF)
> > break;
> >
> > with a variable offset we fall to the TREE_CODE (base) != MEM_REF
> > and will have offset_known == true. Not sure what it does with
> > the result though (it's not the address of a decl).
> >
> > This function seems to oddly special-case != MEM_REF ... (maybe
> > it wants to hande DECL_P () as finishing?
>
> Hmm the function was definitely not written with TARGET_MEM_REF in mind,
> since it was originally used for IPA passes only.
> We basically want to handle stuff like
> >foo
> or
> &(ptr->foo)
> In the second case we want to continue the SSA walk to hopefully work
> out the origin of PTR.
> ipa-modref then looks if the base pointer is derived from function
> parameter or points to local or readonly memory to produce its summary.
> >
> > Note get_addr_base_and_unit_offset will return NULL for
> > a TARGET_MEM_REF <, ..., offset> but TARGET_MEM_REF
> > itself if the base isn't an ADDR_EXPR, irrespective of whether
> > the offset within it is constant or not.
>
> Hmm, interesting. I would expect it to interpret the emantics of TMR
> and return base.
> >
> > Not sure if the above is a problem, but it seems the only caller
> > will just call points_to_local_or_readonly_memory_p on the
> > ADDR_EXPR where refs_local_or_readonly_memory_p via
> > points_to_local_or_readonly_memory_p will eventually do
> >
> > /* See if memory location is clearly invalid. */
> > if (integer_zerop (t))
> > return flag_delete_null_pointer_checks;
> >
> > and that might be a problem. As said, we rely on
> > ADDR_EXPR > to be an address computation
> > that's not subject to strict interpretation to allow IVOPTs
> > doing this kind of optimization w/o introducing some kind of
> > INTEGER_LEA <...>. I know that's a bit awkward but we should
> > make sure this is honored by IPA as well.
> >
> > I'd say
> >
> > diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
> > index 74c9b4e1d1e..45a770cf940 100644
> > --- a/gcc/ipa-fnsummary.cc
> > +++ b/gcc/ipa-fnsummary.cc
> > @@ -2642,7 +2642,8 @@ points_to_local_or_readonly_memory_p (tree t)
> > return true;
> >return !ptr_deref_may_alias_global_p (t, false);
> > }
> > - if (TREE_CODE (t) == ADDR_EXPR)
> > + if (TREE_CODE (t) == ADDR_EXPR
> > + && TREE_CODE (TREE_OPERAND (t, 0)) != TARGET_MEM_REF)
> > return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0));
> >return false;
> > }
> >
> > might eventually work? Alternatively a bit less aggressive like
> > the following.
> >
> > diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
> > index 74c9b4e1d1e..7c79adf6440 100644
> > --- a/gcc/ipa-fnsummary.cc
> > +++ b/gcc/ipa-fnsummary.cc
> > @@ -2642,7 +2642,9 @@ points_to_local_or_readonly_memory_p (tree t)
> > return true;
> >return !ptr_deref_may_alias_global_p (t, false);
> > }
> > - if (TREE_CODE (t) == ADDR_EXPR)
> > + if (TREE_CODE (t) == ADDR_EXPR
> > + && (TREE_CODE (TREE_OPERAND (t, 0)) != TARGET_MEM_REF
> > + || TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) !=
> > INTEGER_CST))
> > return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0));
> >return false;
> > }
>
> Yes, those both looks reasonable to me, perhaps less agressive would be
> better.
Note I didn't check if it helps the testcase ..
> >
> > A "nicer" solution might be to add a informational operand
> > to TARGET_MEM_REF, representing the base pointer to be used for
> > alias/points-to purposes.
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787
--- Comment #17 from Jan Hubicka ---
> > I guess PTA gets around by tracking points-to set also for non-pointer
> > types and consequently it also gives up on any such addition.
>
> It does. But note it does _not_ for POINTER_PLUS where it treats
> the offset operand as non-pointer.
>
> > I think it is ipa-prop.c::unadjusted_ptr_and_unit_offset. It accepts
> > pointer_plus expression, but does not look through POINTER_PLUS.
> > We can restrict it further, but tracking base pointer is quite useful,
> > so it would be nice to not give up completely.
>
> It looks like that function might treat that
>
> ADDR_EXPR >
>
> as integer_zerop base. It does
>
> if (TREE_CODE (op) == ADDR_EXPR)
> {
> poly_int64 extra_offset = 0;
> tree base = get_addr_base_and_unit_offset (TREE_OPERAND (op, 0),
> );
> if (!base)
> {
> base = get_base_address (TREE_OPERAND (op, 0));
> if (TREE_CODE (base) != MEM_REF)
> break;
> offset_known = false;
> }
> else
> {
> if (TREE_CODE (base) != MEM_REF)
> break;
>
> with a variable offset we fall to the TREE_CODE (base) != MEM_REF
> and will have offset_known == true. Not sure what it does with
> the result though (it's not the address of a decl).
>
> This function seems to oddly special-case != MEM_REF ... (maybe
> it wants to hande DECL_P () as finishing?
Hmm the function was definitely not written with TARGET_MEM_REF in mind,
since it was originally used for IPA passes only.
We basically want to handle stuff like
>foo
or
&(ptr->foo)
In the second case we want to continue the SSA walk to hopefully work
out the origin of PTR.
ipa-modref then looks if the base pointer is derived from function
parameter or points to local or readonly memory to produce its summary.
>
> Note get_addr_base_and_unit_offset will return NULL for
> a TARGET_MEM_REF <, ..., offset> but TARGET_MEM_REF
> itself if the base isn't an ADDR_EXPR, irrespective of whether
> the offset within it is constant or not.
Hmm, interesting. I would expect it to interpret the emantics of TMR
and return base.
>
> Not sure if the above is a problem, but it seems the only caller
> will just call points_to_local_or_readonly_memory_p on the
> ADDR_EXPR where refs_local_or_readonly_memory_p via
> points_to_local_or_readonly_memory_p will eventually do
>
> /* See if memory location is clearly invalid. */
> if (integer_zerop (t))
> return flag_delete_null_pointer_checks;
>
> and that might be a problem. As said, we rely on
> ADDR_EXPR > to be an address computation
> that's not subject to strict interpretation to allow IVOPTs
> doing this kind of optimization w/o introducing some kind of
> INTEGER_LEA <...>. I know that's a bit awkward but we should
> make sure this is honored by IPA as well.
>
> I'd say
>
> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
> index 74c9b4e1d1e..45a770cf940 100644
> --- a/gcc/ipa-fnsummary.cc
> +++ b/gcc/ipa-fnsummary.cc
> @@ -2642,7 +2642,8 @@ points_to_local_or_readonly_memory_p (tree t)
> return true;
>return !ptr_deref_may_alias_global_p (t, false);
> }
> - if (TREE_CODE (t) == ADDR_EXPR)
> + if (TREE_CODE (t) == ADDR_EXPR
> + && TREE_CODE (TREE_OPERAND (t, 0)) != TARGET_MEM_REF)
> return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0));
>return false;
> }
>
> might eventually work? Alternatively a bit less aggressive like
> the following.
>
> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
> index 74c9b4e1d1e..7c79adf6440 100644
> --- a/gcc/ipa-fnsummary.cc
> +++ b/gcc/ipa-fnsummary.cc
> @@ -2642,7 +2642,9 @@ points_to_local_or_readonly_memory_p (tree t)
> return true;
>return !ptr_deref_may_alias_global_p (t, false);
> }
> - if (TREE_CODE (t) == ADDR_EXPR)
> + if (TREE_CODE (t) == ADDR_EXPR
> + && (TREE_CODE (TREE_OPERAND (t, 0)) != TARGET_MEM_REF
> + || TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) !=
> INTEGER_CST))
> return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0));
>return false;
> }
Yes, those both looks reasonable to me, perhaps less agressive would be
better.
>
> A "nicer" solution might be to add a informational operand
> to TARGET_MEM_REF, representing the base pointer to be used for
> alias/points-to purposes. But if that's not invariant it might
> keep some otherwise unnecessary definition stmts live.
Yep, I see that forcing extra IV to track original semantics would be
trouble here. I think that after iv-opts we should be done with more
fancy propagation across loops.
However, to avoid ipa-modref summary degradation, perhaps scheduling the
pass before ivopts would make sense...
Thanks,
Honza
Re: [Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
> > I guess PTA gets around by tracking points-to set also for non-pointer
> > types and consequently it also gives up on any such addition.
>
> It does. But note it does _not_ for POINTER_PLUS where it treats
> the offset operand as non-pointer.
>
> > I think it is ipa-prop.c::unadjusted_ptr_and_unit_offset. It accepts
> > pointer_plus expression, but does not look through POINTER_PLUS.
> > We can restrict it further, but tracking base pointer is quite useful,
> > so it would be nice to not give up completely.
>
> It looks like that function might treat that
>
> ADDR_EXPR >
>
> as integer_zerop base. It does
>
> if (TREE_CODE (op) == ADDR_EXPR)
> {
> poly_int64 extra_offset = 0;
> tree base = get_addr_base_and_unit_offset (TREE_OPERAND (op, 0),
> );
> if (!base)
> {
> base = get_base_address (TREE_OPERAND (op, 0));
> if (TREE_CODE (base) != MEM_REF)
> break;
> offset_known = false;
> }
> else
> {
> if (TREE_CODE (base) != MEM_REF)
> break;
>
> with a variable offset we fall to the TREE_CODE (base) != MEM_REF
> and will have offset_known == true. Not sure what it does with
> the result though (it's not the address of a decl).
>
> This function seems to oddly special-case != MEM_REF ... (maybe
> it wants to hande DECL_P () as finishing?
Hmm the function was definitely not written with TARGET_MEM_REF in mind,
since it was originally used for IPA passes only.
We basically want to handle stuff like
>foo
or
&(ptr->foo)
In the second case we want to continue the SSA walk to hopefully work
out the origin of PTR.
ipa-modref then looks if the base pointer is derived from function
parameter or points to local or readonly memory to produce its summary.
>
> Note get_addr_base_and_unit_offset will return NULL for
> a TARGET_MEM_REF <, ..., offset> but TARGET_MEM_REF
> itself if the base isn't an ADDR_EXPR, irrespective of whether
> the offset within it is constant or not.
Hmm, interesting. I would expect it to interpret the emantics of TMR
and return base.
>
> Not sure if the above is a problem, but it seems the only caller
> will just call points_to_local_or_readonly_memory_p on the
> ADDR_EXPR where refs_local_or_readonly_memory_p via
> points_to_local_or_readonly_memory_p will eventually do
>
> /* See if memory location is clearly invalid. */
> if (integer_zerop (t))
> return flag_delete_null_pointer_checks;
>
> and that might be a problem. As said, we rely on
> ADDR_EXPR > to be an address computation
> that's not subject to strict interpretation to allow IVOPTs
> doing this kind of optimization w/o introducing some kind of
> INTEGER_LEA <...>. I know that's a bit awkward but we should
> make sure this is honored by IPA as well.
>
> I'd say
>
> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
> index 74c9b4e1d1e..45a770cf940 100644
> --- a/gcc/ipa-fnsummary.cc
> +++ b/gcc/ipa-fnsummary.cc
> @@ -2642,7 +2642,8 @@ points_to_local_or_readonly_memory_p (tree t)
> return true;
>return !ptr_deref_may_alias_global_p (t, false);
> }
> - if (TREE_CODE (t) == ADDR_EXPR)
> + if (TREE_CODE (t) == ADDR_EXPR
> + && TREE_CODE (TREE_OPERAND (t, 0)) != TARGET_MEM_REF)
> return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0));
>return false;
> }
>
> might eventually work? Alternatively a bit less aggressive like
> the following.
>
> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
> index 74c9b4e1d1e..7c79adf6440 100644
> --- a/gcc/ipa-fnsummary.cc
> +++ b/gcc/ipa-fnsummary.cc
> @@ -2642,7 +2642,9 @@ points_to_local_or_readonly_memory_p (tree t)
> return true;
>return !ptr_deref_may_alias_global_p (t, false);
> }
> - if (TREE_CODE (t) == ADDR_EXPR)
> + if (TREE_CODE (t) == ADDR_EXPR
> + && (TREE_CODE (TREE_OPERAND (t, 0)) != TARGET_MEM_REF
> + || TREE_CODE (TREE_OPERAND (TREE_OPERAND (t, 0), 0)) !=
> INTEGER_CST))
> return refs_local_or_readonly_memory_p (TREE_OPERAND (t, 0));
>return false;
> }
Yes, those both looks reasonable to me, perhaps less agressive would be
better.
>
> A "nicer" solution might be to add a informational operand
> to TARGET_MEM_REF, representing the base pointer to be used for
> alias/points-to purposes. But if that's not invariant it might
> keep some otherwise unnecessary definition stmts live.
Yep, I see that forcing extra IV to track original semantics would be
trouble here. I think that after iv-opts we should be done with more
fancy propagation across loops.
However, to avoid ipa-modref summary degradation, perhaps scheduling the
pass before ivopts would make sense...
Thanks,
Honza
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787
--- Comment #16 from rguenther at suse dot de ---
On Tue, 13 Feb 2024, hubicka at ucw dot cz wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787
>
> --- Comment #15 from Jan Hubicka ---
> >
> > IVOPTs does the above but it does it (or should) as
> >
> > offset = (uintptr) - (uintptr)
> > val = *((T *)((uintptr)base1 + i + offset))
> >
> > which is OK for points-to as no POINTER_PLUS_EXPR is involved so the
> > resulting pointer points to both base1 and base2 (which isn't optimal
> > but correct).
> >
> > If we somehow get back a POINTER_PLUS that's where things go wrong.
> >
> > Doing the above in C code would be valid input so we have to treat
> > it correctly (OK, the standard only allows back-and-forth
> > pointer-to-integer casts w/o any adjustment, but of course we relax
> > this).
>
> OK. Modrefs tracks base pointer for accesses and tries to prove that
> they are function parameters. This should immitate ivopts:
> void
> __attribute__ ((noinline))
> set(int *a, unsigned long off)
> {
> *(int *)((unsigned long)a + off) = 1;
> }
> int
> test ()
> {
> int a;
> int b = 0;
> set (, (unsigned long) - (unsigned long));
> return b;
> }
>
> Here set gets following gimple at modref2 time:
> __attribute__((noinline))
> void set (int * a, long unsigned int off)
> {
> long unsigned int a.0_1;
> long unsigned int _2;
> int * _3;
>
>[local count: 1073741824]:
> a.0_1 = (long unsigned int) a_4(D);
> _2 = a.0_1 + off_5(D);
> _3 = (int *) _2;
> *_3 = 1;
> return;
>
> }
>
> This is not pattern matched so modref does not think the access has a as
> a base:
>
> stores:
> Base 0: alias set 1
> Ref 0: alias set 1
> Every access
>
> While for:
>
> void
> __attribute__ ((noinline))
> set(int *a, unsigned long off)
> {
> *(a+off/sizeof(int))=1;
> }
>
> we produce:
>
> __attribute__((noinline))
> void set (int * a, long unsigned int off)
> {
> sizetype _1;
> int * _2;
>
>[local count: 1073741824]:
> _1 = off_3(D) & 18446744073709551612;
> _2 = a_4(D) + _1;
> *_2 = 1;
> return;
> }
>
> And this is understood:
>
> stores:
> Base 0: alias set 1
> Ref 0: alias set 1
> access: Parm 0
>
> If we consider it correct to optimize out the conversion from and to
> pointer type, then I suppose any addition of pointer and integer which
> we do not see means that we need to give up on tracking base completely.
>
> I guess PTA gets around by tracking points-to set also for non-pointer
> types and consequently it also gives up on any such addition.
>
> But what we really get from relaxing this?
> >
> > IVOPTs then in putting all of the stuff into 'offset' gets at
> > trying a TARGET_MEM_REF based on a NULL base but that's invalid.
> > We then resort to a LEA (ADDR_EXPR of TARGET_MEM_REF) to compute
> > the address which gets us into some phishy argument that it's
> > not valid to decompose ADDR_EXPR of TARGET_MEM_REF to
> > POINTER_PLUS of the TARGET_MEM_REF base and the offset. But
> > that's how it is (points-to treats (address of) TARGET_MEM_REF
> > as pointing to anything ...).
> >
> > > A quick fix would be to run IPA modref before ivopts, but I do not see
> > > how such
> > > transformation can work with rest of alias analysis (PTA etc)
> >
> > It does. Somewhere IPA modref interprets things wrongly, I didn't figure
> > out here though.
>
>
> I guess PTA gets around by tracking points-to set also for non-pointer
> types and consequently it also gives up on any such addition.
It does. But note it does _not_ for POINTER_PLUS where it treats
the offset operand as non-pointer.
> I think it is ipa-prop.c::unadjusted_ptr_and_unit_offset. It accepts
> pointer_plus expression, but does not look through POINTER_PLUS.
> We can restrict it further, but tracking base pointer is quite useful,
> so it would be nice to not give up completely.
It looks like that function might treat that
ADDR_EXPR >
as integer_zerop base. It does
if (TREE_CODE (op) == ADDR_EXPR)
{
poly_int64 extra_offset = 0;
tree base = get_addr_base_and_unit_offset (TREE_OPERAND (op, 0),
);
if (!base)
{
base = get_base_address (TREE_OPERAND (op, 0));
if (TREE_CODE (base) != MEM_REF)
break;
offset_known = false;
}
else
{
if (TREE_CODE (base) != MEM_REF)
break;
with a variable offset we fall to the TREE_CODE (base) != MEM_REF
and will have offset_known == true. Not sure what it does with
the result though (it's not the address of a decl).
This function seems to oddly special-case != MEM_REF ... (maybe
it wants to hande DECL_P () as finishing?
Note get_addr_base_and_unit_offset will return NULL for
a TARGET_MEM_REF <, ..., offset> but
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787
--- Comment #15 from Jan Hubicka ---
>
> IVOPTs does the above but it does it (or should) as
>
> offset = (uintptr) - (uintptr)
> val = *((T *)((uintptr)base1 + i + offset))
>
> which is OK for points-to as no POINTER_PLUS_EXPR is involved so the
> resulting pointer points to both base1 and base2 (which isn't optimal
> but correct).
>
> If we somehow get back a POINTER_PLUS that's where things go wrong.
>
> Doing the above in C code would be valid input so we have to treat
> it correctly (OK, the standard only allows back-and-forth
> pointer-to-integer casts w/o any adjustment, but of course we relax
> this).
OK. Modrefs tracks base pointer for accesses and tries to prove that
they are function parameters. This should immitate ivopts:
void
__attribute__ ((noinline))
set(int *a, unsigned long off)
{
*(int *)((unsigned long)a + off) = 1;
}
int
test ()
{
int a;
int b = 0;
set (, (unsigned long) - (unsigned long));
return b;
}
Here set gets following gimple at modref2 time:
__attribute__((noinline))
void set (int * a, long unsigned int off)
{
long unsigned int a.0_1;
long unsigned int _2;
int * _3;
[local count: 1073741824]:
a.0_1 = (long unsigned int) a_4(D);
_2 = a.0_1 + off_5(D);
_3 = (int *) _2;
*_3 = 1;
return;
}
This is not pattern matched so modref does not think the access has a as
a base:
stores:
Base 0: alias set 1
Ref 0: alias set 1
Every access
While for:
void
__attribute__ ((noinline))
set(int *a, unsigned long off)
{
*(a+off/sizeof(int))=1;
}
we produce:
__attribute__((noinline))
void set (int * a, long unsigned int off)
{
sizetype _1;
int * _2;
[local count: 1073741824]:
_1 = off_3(D) & 18446744073709551612;
_2 = a_4(D) + _1;
*_2 = 1;
return;
}
And this is understood:
stores:
Base 0: alias set 1
Ref 0: alias set 1
access: Parm 0
If we consider it correct to optimize out the conversion from and to
pointer type, then I suppose any addition of pointer and integer which
we do not see means that we need to give up on tracking base completely.
I guess PTA gets around by tracking points-to set also for non-pointer
types and consequently it also gives up on any such addition.
But what we really get from relaxing this?
>
> IVOPTs then in putting all of the stuff into 'offset' gets at
> trying a TARGET_MEM_REF based on a NULL base but that's invalid.
> We then resort to a LEA (ADDR_EXPR of TARGET_MEM_REF) to compute
> the address which gets us into some phishy argument that it's
> not valid to decompose ADDR_EXPR of TARGET_MEM_REF to
> POINTER_PLUS of the TARGET_MEM_REF base and the offset. But
> that's how it is (points-to treats (address of) TARGET_MEM_REF
> as pointing to anything ...).
>
> > A quick fix would be to run IPA modref before ivopts, but I do not see how
> > such
> > transformation can work with rest of alias analysis (PTA etc)
>
> It does. Somewhere IPA modref interprets things wrongly, I didn't figure
> out here though.
I guess PTA gets around by tracking points-to set also for non-pointer
types and consequently it also gives up on any such addition.
I think it is ipa-prop.c::unadjusted_ptr_and_unit_offset. It accepts
pointer_plus expression, but does not look through POINTER_PLUS.
We can restrict it further, but tracking base pointer is quite useful,
so it would be nice to not give up completely.
Honza
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 --- Comment #14 from rguenther at suse dot de --- On Tue, 13 Feb 2024, hubicka at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 > > --- Comment #13 from Jan Hubicka --- > So my understanding is that ivopts does something like > > offset = - > > and then translate > val = base2[i] > to > val = *((base1+i)+offset) > > Where (base1+i) is then an iv variable. > > I wonder if we consider doing memory reference with base changed via offset a > valid transformation. Is there way to tell when this happens? IVOPTs does the above but it does it (or should) as offset = (uintptr) - (uintptr) val = *((T *)((uintptr)base1 + i + offset)) which is OK for points-to as no POINTER_PLUS_EXPR is involved so the resulting pointer points to both base1 and base2 (which isn't optimal but correct). If we somehow get back a POINTER_PLUS that's where things go wrong. Doing the above in C code would be valid input so we have to treat it correctly (OK, the standard only allows back-and-forth pointer-to-integer casts w/o any adjustment, but of course we relax this). IVOPTs then in putting all of the stuff into 'offset' gets at trying a TARGET_MEM_REF based on a NULL base but that's invalid. We then resort to a LEA (ADDR_EXPR of TARGET_MEM_REF) to compute the address which gets us into some phishy argument that it's not valid to decompose ADDR_EXPR of TARGET_MEM_REF to POINTER_PLUS of the TARGET_MEM_REF base and the offset. But that's how it is (points-to treats (address of) TARGET_MEM_REF as pointing to anything ...). > A quick fix would be to run IPA modref before ivopts, but I do not see how > such > transformation can work with rest of alias analysis (PTA etc) It does. Somewhere IPA modref interprets things wrongly, I didn't figure out here though.
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 --- Comment #13 from Jan Hubicka --- So my understanding is that ivopts does something like offset = - and then translate val = base2[i] to val = *((base1+i)+offset) Where (base1+i) is then an iv variable. I wonder if we consider doing memory reference with base changed via offset a valid transformation. Is there way to tell when this happens? A quick fix would be to run IPA modref before ivopts, but I do not see how such transformation can work with rest of alias analysis (PTA etc)
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787
--- Comment #12 from Alex Coplan ---
Here is an alternative testcase that also fails in the same way on the GCC 12
and 13 branches:
void foo(int x, int y, int z, int d, int *buf)
{
for(int i = z; i < y-z; ++i)
for(int j = 0; j < d; ++j)
buf[i*x+(z-j-1)] = buf[i*x+(z+j)];
}
void bar(int x, int y, int z, int d, int *buf)
{
for(int i = 0; i < d; ++i)
for(int j = z; j < x-z; ++j)
buf[j+(z-i-1)*x] = buf[j+(z+i)*x];
}
__attribute__((noipa))
void baz(int x, int y, int d, int *buf)
{
foo(x, y, 0, d, buf);
bar(x, y, 0, d, buf);
}
int main(void)
{
int a[] = { 1, 2, 3 };
baz (1, 2, 1, a+1);
/* buf = a+1.
foo does:
buf[-1] = buf[0]; // { 2, 2, 3 }
buf[0] = buf[1]; // { 2, 3, 3 }
bar does:
buf[-1] = buf[0]; // { 3, 3, 3 } */
for (int i = 0; i < 2; i++)
if (a[i] != 3)
__builtin_abort ();
}
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 --- Comment #11 from Richard Biener --- Btw, there's related IPA modref wrong-code issues where IPA and late summaries are merged incorrectly (also receiving no attention)
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 Richard Biener changed: What|Removed |Added Target||aarch64 --- Comment #10 from Richard Biener --- I think it's ipa-modref analyze_store bailing for if (a.parm_index == MODREF_LOCAL_MEMORY_PARM) return false; no idea how it arrives at that.
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 --- Comment #9 from Andrew Pinski --- _57 = [(int *)0B + _56 + _55 * 1]; *_57 = _14; The fix for PR 110702 must not have been enough. Or rather this part of the explanation was fully true: ``` The patch below recognizes the fallback after the fact and transforms the TARGET_MEM_REF memory reference into a LEA for which this form isn't problematic: _24 = [(long int *)0B + ivtmp.36_34 + ivtmp.28_44 * 4]; _3 = *_24; ``` Maybe it was enough for GCC 13/12 branches but the trunk it was not.
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 --- Comment #8 from Jan Hubicka --- I will take a look. Mod-ref only reuses the code detecting errneous paths in ssa-split-paths, so that code will get confused, too. It makes sense for ivopts to compute difference of two memory allocations, but I wonder if that won't also confuse PTA and other stuff, so perhaps we need way to exlicitely tag memory location where such optimization happen? (to make it clear that original base is lost, or keep track of it)
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 --- Comment #7 from Alex Coplan --- (In reply to Andrew Pinski from comment #6) > (In reply to Jakub Jelinek from comment #5) > > My bisection points to r12-5915-ge93809f62363ba4b233858005aef652fb550e896 > > Which means it is related to bug 110702 . > > Again try -fno-ivopts . I suspect ivopts is producing some odd ir that is > confusing modref here. Yeah, it seems -fno-ivopts makes the execution test pass too (so -O -fno-ivopts).
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 --- Comment #6 from Andrew Pinski --- (In reply to Jakub Jelinek from comment #5) > My bisection points to r12-5915-ge93809f62363ba4b233858005aef652fb550e896 Which means it is related to bug 110702 . Again try -fno-ivopts . I suspect ivopts is producing some odd ir that is confusing modref here.
[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787 Jakub Jelinek changed: What|Removed |Added Last reconfirmed||2024-02-06 Status|UNCONFIRMED |NEW Ever confirmed|0 |1 Summary|[14 Regression] Wrong code |[12/13/14 Regression] Wrong |at -O with ipa-modref on|code at -O with ipa-modref |aarch64 |on aarch64 Priority|P3 |P2 Target Milestone|--- |12.4 --- Comment #5 from Jakub Jelinek --- My bisection points to r12-5915-ge93809f62363ba4b233858005aef652fb550e896
