[Bug tree-optimization/103117] New: uncprop produces harder to analyze but not better code

[Bug tree-optimization/103117] New: uncprop produces harder to analyze but not better code

[hubicka at gcc dot gnu.org]

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

            Bug ID: 103117
           Summary: uncprop produces harder to analyze but not better code
           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: ---

For the following
struct a {int v; struct a *next;};

struct a*
next(struct a *a)
{
  if (!a || a->v)
    return 0;
  return a->next;
}

(we have quite few functions like this in gcc sources)

We produce

struct a * next (struct a * a)
{
  int _1;
  struct a * _2;
  struct a * _5;

  <bb 2> [local count: 1073741824]:
  if (a_3(D) == 0B)
    goto <bb 6>; [8.27%]
  else
    goto <bb 3>; [91.73%]

  <bb 6> [local count: 88798448]:
  goto <bb 5>; [100.00%]

  <bb 3> [local count: 984943377]:
  _1 = a_3(D)->v;
  if (_1 != 0)
    goto <bb 7>; [17.38%]
  else
    goto <bb 4>; [82.62%]

  <bb 7> [local count: 171183158]:
  goto <bb 5>; [100.00%]

  <bb 4> [local count: 813760219]:
  _5 = a_3(D)->next;

  <bb 5> [local count: 1073741824]:
  # _2 = PHI <0B(7), _5(4), 0B(6)>
  return _2;

}

while uncprop concludes it is a good idea to turn 0 in the last PHI to a:

  # _2 = PHI <0B(7), _5(4), a_3(D)(6)>

this confuses e.g. modref that thinks that a may be returned directly because
it does not understand that the a_3 argument of PHI is funny way to express
constant 0.

I do not think the uncprop change helps anything - the PHI still has 0 argument
in the other arm.

The modref confussion can be solved by moving it before uncprop, but I wonder
if we want to do the transform after all.

[Bug tree-optimization/103117] uncprop produces harder to analyze but not better code

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
uncprop is supposed to reduce the number of copies required when expanding PHIs
via coalescing which is never possible for constant PHI arguments, so yes,
we do want this and maybe also for the case in question split out a forwarder
to have only a single set of zero.

I suppose modref could (for pointer returns) use ranger to query its range
and see if it ever is non-NULL?  I'm not sure if we reliably propagate
null pointer constants everywhere.

Btw, uncprop is supposed to run right before RTL expansion - it is in fact
an out-of-SSA optimization, so even removing it as separate pass and
directly calling it from rewrite_out_of_ssa after eliminate_useless_phis
might be an improvement.

[Bug tree-optimization/103117] uncprop produces harder to analyze but not better code

[hubicka at kam dot mff.cuni.cz]

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

--- Comment #2 from hubicka at kam dot mff.cuni.cz ---
> I suppose modref could (for pointer returns) use ranger to query its range
> and see if it ever is non-NULL?  I'm not sure if we reliably propagate
> null pointer constants everywhere.

I think we simply want to reorder the passes here...
> 
> Btw, uncprop is supposed to run right before RTL expansion - it is in fact
> an out-of-SSA optimization, so even removing it as separate pass and
> directly calling it from rewrite_out_of_ssa after eliminate_useless_phis
> might be an improvement.

I don't know - this way we have separate dumps etc.  I think mistake was
scheduling pure-const and later modref too late.

Thanks,
Honza

[Bug tree-optimization/103117] uncprop produces harder to analyze but not better code

[rguenther at suse dot de]

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

--- Comment #3 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 8 Nov 2021, hubicka at kam dot mff.cuni.cz wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103117
> 
> --- Comment #2 from hubicka at kam dot mff.cuni.cz ---
> > I suppose modref could (for pointer returns) use ranger to query its range
> > and see if it ever is non-NULL?  I'm not sure if we reliably propagate
> > null pointer constants everywhere.
> 
> I think we simply want to reorder the passes here...
> > 
> > Btw, uncprop is supposed to run right before RTL expansion - it is in fact
> > an out-of-SSA optimization, so even removing it as separate pass and
> > directly calling it from rewrite_out_of_ssa after eliminate_useless_phis
> > might be an improvement.
> 
> I don't know - this way we have separate dumps etc.  I think mistake was
> scheduling pure-const and later modref too late.

Maybe.  If you move them please put a comment before uncprop that it
should be last before RTL expansion.

[Bug tree-optimization/103117] uncprop produces harder to analyze but not better code

[hubicka at kam dot mff.cuni.cz]

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

--- Comment #4 from hubicka at kam dot mff.cuni.cz ---
> > I don't know - this way we have separate dumps etc.  I think mistake was
> > scheduling pure-const and later modref too late.
> 
> Maybe.  If you move them please put a comment before uncprop that it
> should be last before RTL expansion.
Will do, thanks!
Honza

[Bug tree-optimization/103117] uncprop produces harder to analyze but not better code

[hubicka at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #5 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
As discussed uncprop really should propagate this way :)