[Bug tree-optimization/113787] [12/13/14 Regression] Wrong code at -O with ipa-modref on aarch64

["rguenther at suse dot de" <gcc-bugzilla@gcc.gnu.org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113787

--- Comment #18 from rguenther at suse dot de <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 <hubicka at ucw dot cz> ---
> > > 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 <TARGET_MEM_REF <0B, ...>>
> > 
> > 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),
> >                                                      &offset);
> >           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
>  &decl->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 <&decl, ..., 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 <TARGET_MEM_REF <...> > 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.  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 ...