[Bug tree-optimization/103223] New: [12 regression] Access attribute prevents IPA optimization

[Bug tree-optimization/103223] New: [12 regression] Access attribute prevents IPA optimization

[hubicka at gcc dot gnu.org]

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

            Bug ID: 103223
           Summary: [12 regression] Access attribute prevents IPA
                    optimization
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hubicka at gcc dot gnu.org
  Target Milestone: ---

Hi,
ipa-fnsummary sets can_change_signature flag which determines whether we can
manipulate parameters of a given function.  It tests:

       /* Type attributes can use parameter indices to describe them.  */
       if (TYPE_ATTRIBUTES (TREE_TYPE (node->decl))
         node->can_change_signature = false
Which unfortunately triggers on many C functions now when we introduced the
access attribute.

Updating happens in ipa-param-manipulation and we do have infrastructure how to
rewrite (suriving) old attributes to new ones, so we could support access
attribute updating (or always map to old indexes when using it).

I don't think possible warnings should inhibit useful optimizations and this is
a regression wrt compilers before the access attribute.  I am having patch to
fix similar issue with fnspec attribute that can be safely removed at signature
change since we now can preserve info in ipa-modref.

Martin, I wonder if if you would be OK with simply dropping the access when
function signature changes (which I can prepare patch for) or do you want to
dive into updating it?

Once new fuction is created, for every new parameter there is
get_original_index accessor which returns original parameter index (if it
exists).  This could be easily used to update access and drop those entries
that was really optimized out IMO

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[hubicka at gcc dot gnu.org]

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

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[12 regression] Access      |[12 regression] Access
                   |attribute prevents IPA      |attribute dropped when
                   |optimization                |ipa-sra is applied

--- Comment #1 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
We now drop the access attribute. Hope it is not too bad information loss.

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0
           Keywords|                            |diagnostic
   Last reconfirmed|                            |2021-11-15
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |WAITING
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Waiting for a testcase where the effect is visible.

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=100491

--- Comment #3 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Isn't this a duplicate of PR 100491?

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[jamborm at gcc dot gnu.org]

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

--- Comment #4 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #0)
> Hi,
> ipa-fnsummary sets can_change_signature flag which determines whether we can
> manipulate parameters of a given function.  It tests:
> 
>        /* Type attributes can use parameter indices to describe them.  */
>        if (TYPE_ATTRIBUTES (TREE_TYPE (node->decl))
>          node->can_change_signature = false
> Which unfortunately triggers on many C functions now when we introduced the
> access attribute.
> 
> Updating happens in ipa-param-manipulation and we do have infrastructure how
> to rewrite (suriving) old attributes to new ones, so we could support access
> attribute updating (or always map to old indexes when using it).

We do?  I thought I would need to write it (together with recognizing
parameters which we can safely update/ignore).

> 
> I don't think possible warnings should inhibit useful optimizations and this
> is a regression wrt compilers before the access attribute.  I am having
> patch to fix similar issue with fnspec attribute that can be safely removed
> at signature change since we now can preserve info in ipa-modref.
> 
> Martin, I wonder if if you would be OK with simply dropping the access when
> function signature changes (which I can prepare patch for) or do you want to
> dive into updating it?

I would be OK with it but I don't think people who invested the energy
into these new security warnings would.

> 
> Once new fuction is created, for every new parameter there is
> get_original_index accessor which returns original parameter index (if it
> exists).  This could be easily used to update access and drop those entries
> that was really optimized out IMO

Yeah.  I guess that is the necessary thing to do.

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[msebor at gcc dot gnu.org]

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

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Martin Jambor from comment #4)
> (In reply to Jan Hubicka from comment #0)
...
> > Martin, I wonder if if you would be OK with simply dropping the access when
> > function signature changes (which I can prepare patch for) or do you want to
> > dive into updating it?
> 
> I would be OK with it but I don't think people who invested the energy
> into these new security warnings would.

I replied earlier on gcc-patches: I've always intended the access attribute to
eventually benefit optimization so please feel free (and encouraged :) to use
it
for that purpose.  The idea behind it was not just to catch bugs but also to
enable optimizations based on the expectation that those bugs will have been
fixed.  (This has to be done carefully since the attribute is also implicitly
added in contexts where relying on it wouldn't correct for optimization; the
attirbute API makes it possible to distinguish these cases.)

By dropping the attribute in IPA passes we would not only give up on detecting
the bugs the IPA transformations expose but also on the optimization
opportunities they might open up.

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[jamborm at gcc dot gnu.org]

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

--- Comment #6 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #5)
> I replied earlier on gcc-patches: I've always intended the access attribute
> to eventually benefit optimization so please feel free (and encouraged :) to
> use it
> for that purpose.  The idea behind it was not just to catch bugs but also to
> enable optimizations based on the expectation that those bugs will have been
> fixed.  (This has to be done carefully since the attribute is also
> implicitly added in contexts where relying on it wouldn't correct for
> optimization; the attirbute API makes it possible to distinguish these
> cases.)
> 
> By dropping the attribute in IPA passes we would not only give up on
> detecting the bugs the IPA transformations expose but also on the
> optimization opportunities they might open up.

Right.  Can you please help us write code that creates access attribute from
another one after parameters at given indices are dropped?

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[msebor at gcc dot gnu.org]

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

--- Comment #7 from Martin Sebor <msebor at gcc dot gnu.org> ---
For an attribute access that's explicitly specified on the declaration of a
function I think these steps should work:

1) Call init_attr_rdwr_indices() to initialize the mapping for the original
function with type fntype:

  rdwr_map rdwr_idx;
  init_attr_rdwr_indices (&rdwr_idx, TYPE_ATTRIBUTES (fntype));

2) Iterate over the mapping, rdwr_idx, creating an attribute access for each
pair of function arguments coupled by the attribute (i.e., pointer and size)
that also appears in the cloned function, and dropping those that don't. 
Append each specification to a chain of attributes.

3) Call decl_attributes() on the chain.

So maybe an API like this:

  bool copy_attr_access (tree newdecl, tree decl_to_clone, bitmap
args_to_copy);

where newdecl is the function cloned from the original decl_to_clone but with
fewer arguments and args_to_copy is a bitmap of the positions of the
decl_to_clone arguments for which to copy the access spec?  (Or, alternatively,
args_to_drop.)  Would this work for what you're thinking of using it for?  

This approach won't work for an explicitly specified attribute access where IPA
substitutes a constant for the bound, as in:

  __attribute__ ((access (write_only, 1, 2))) void f (int *a, int n) { ... }

when transformed into with 7 being the value of n:

  void f.constprop.7 (int *a) { ... }

Ideally, we want to change the cloned function to this:

  void f.constprop.7 (int a[7]) { ... }

so that calls it with fewer than 7 elements are diagnosed.  But there's no way
to specify __attribute__ ((access)) like that.  That's done internally, in
stages, by the front end by adding an "arg spec" attribute to the argument a as
it's seen in the argument list.  Once the argument list is fully processed the
front end then creates the attribute access specification for the function from
the "arg spec" attributes on all the arguments.  We need a way to do the same
thing in the middle end.

if he above is close, can you show me where you would call the function from?

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[hubicka at kam dot mff.cuni.cz]

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

--- Comment #8 from hubicka at kam dot mff.cuni.cz ---
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103223
> 
> --- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
> (In reply to Martin Jambor from comment #4)
> > (In reply to Jan Hubicka from comment #0)
> ...
> > > Martin, I wonder if if you would be OK with simply dropping the access when
> > > function signature changes (which I can prepare patch for) or do you want to
> > > dive into updating it?
> > 
> > I would be OK with it but I don't think people who invested the energy
> > into these new security warnings would.
> 
> I replied earlier on gcc-patches: I've always intended the access attribute to
> eventually benefit optimization so please feel free (and encouraged :) to use
> it
> for that purpose.  The idea behind it was not just to catch bugs but also to
> enable optimizations based on the expectation that those bugs will have been
> fixed.  (This has to be done carefully since the attribute is also implicitly
> added in contexts where relying on it wouldn't correct for optimization; the
> attirbute API makes it possible to distinguish these cases.)

Is there some summary of what information the access attribute holds
in addition to what we have in fnspec (which is used for the
optimization)?
> 
> By dropping the attribute in IPA passes we would not only give up on detecting
> the bugs the IPA transformations expose but also on the optimization
> opportunities they might open up.

Yep, I hope it is not too hard to write code updating the attribute
after transformation.  We drop the attribute only when we actually do
some change on function parameters.

ipa-modref solves similar problem of updating the modref summaries after
changes.  What I do is to simply compute map translating old parameter
indexes to new via:

  clone_info *info = clone_info::get (node);

  FOR_EACH_VEC_SAFE_ELT (info->param_adjustments->m_adj_params, i, p)
    {
      int idx = info->param_adjustments->get_original_index (i);
      if (idx > (int)max)
        max = idx;
    }

  auto_vec <int, 32> map;

  map.reserve (max + 1);
  for (i = 0; i <= max; i++)
    map.quick_push (-1);
  FOR_EACH_VEC_SAFE_ELT (info->param_adjustments->m_adj_params, i, p)
    {
      int idx = info->param_adjustments->get_original_index (i);
      if (idx >= 0)
        map[idx] = i;
    }

and then I simply rewrite old parameter indexes to new and drop parts of
info that uses parameters that was either removed or replaced by
scalars/components.

Similarly we can do for access attribute.

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[msebor at gcc dot gnu.org]

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

--- Comment #9 from Martin Sebor <msebor at gcc dot gnu.org> ---
Outside of code comments only the user-visible attribute access interface is
documented in the manual.

The main difference is probably that attribute access can be added by the user
for any pair of arguments (without a limit on the number of arguments). 
Another difference is that the implicit attribute access captures the VLA
argument bounds and the [static N] specifier that tells us that array must have
at least N elements (this latter part could be used for optimization).  The
attribute access mode none means that the argument isn't dereferenced but must
point to an object of the specified size (I'm guessing that's not quite the
same as the 'x' character in fn spec).  For the common subset it might make
sense to transform attribute access to fnspec when it applies to the first N
arguments (with N being the fnspec limit).

Come to think of it, an alternate approach to solving the IPA problem with lost
attribute access might be to extend attr fnspec to encode the same things as it
and for the warnings to use fnspec instead.  The same caveat applies that not
all of what attribute access describes can be used for optimization, so the API
would have to make the distinction quite clear.

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[admin at levyhsu dot com]

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

Levy <admin at levyhsu dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |admin at levyhsu dot com

--- Comment #10 from Levy <admin at levyhsu dot com> ---
Hi Jan

Just want provide a status report that this commit
ecdf414bd89e6ba251f6b3f494407139b4dbae0e seems to caused about 50% regression
when running multi-copy 548.exchange2_r with march_native_ofast_lto on
spec2017:

Xeon(R) Platinum 8358 (IceLake) (64C 128T 512G):
BenchMarks      Copies  RunTime1        RunTime2        Rate1   Rate2   Compare
548.exchange2_r 128     479     913     700     367     -47.57%

Xeon(R) Gold 6252 (CascadeLake) (48C 96T 192G)
BenchMarks      Copies  RunTime1        RunTime2        Rate1   Rate2   Compare
548.exchange2_r 96      643     1240    391     203     -48.08%

Best
Levy

[Bug tree-optimization/103223] [12 regression] Access attribute dropped when ipa-sra is applied

[hubicka at kam dot mff.cuni.cz]

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

--- Comment #11 from hubicka at kam dot mff.cuni.cz ---
> Xeon(R) Platinum 8358 (IceLake) (64C 128T 512G):
> BenchMarks      Copies  RunTime1        RunTime2        Rate1   Rate2   Compare
> 548.exchange2_r 128     479     913     700     367     -47.57%
> 
> Xeon(R) Gold 6252 (CascadeLake) (48C 96T 192G)
> BenchMarks      Copies  RunTime1        RunTime2        Rate1   Rate2   Compare
> 548.exchange2_r 96      643     1240    391     203     -48.08%

I filled in PR103227 to track this problem.  There seems to be two
issues visible on exchange2.  First is that ipa-sra changes order of
functions which in which inliner visits them and this makes difference
in inlining decisions. Second is that ipa-sra makes some constant
propagation info to be lost.  With Martin we look into this.

[Bug ipa/103223] [12/13 regression] Access attribute dropped when ipa-sra is applied

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.0                        |12.2

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 12.1 is being released, retargeting bugs to GCC 12.2.

[Bug ipa/103223] [12/13/14 regression] Access attribute dropped when ipa-sra is applied

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.3                        |12.4

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 12.3 is being released, retargeting bugs to GCC 12.4.