[Bug rtl-optimization/64164] [4.9/5 Regression] one more stack slot used due to one less inlining level

[rguenth at gcc dot gnu.org]

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

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Alexandre Oliva from comment #9)
> (In reply to Jeffrey A. Law from comment #7)
> > OK, but why does this make such a difference in the final result.  Ie, what happens as we get into RTL.
> 
> Err, I covered that bit in my description: we emit a number of copies, each
> with a new BB of its own.  The additional pseudo survives all the way to the
> end, and so do the copies, which is enough to explain the additional stack
> slot.  Further rearrangement of code also follows from the additional blocks.
> 
> (In reply to Andrew Macleod from comment #8)
> > most of these kinds of restrictions were rooted in some kind of rationale... usually from examining an issue of some sort and introducing the restriction to avoid it.
> 
> This one was added by
> https://gcc.gnu.org/ml/gcc-patches/2012-08/msg00517.html
> The description mentions out-of-SSA coalescing, but not copyrename
> coalescing, which is what this is about.  Since there were not anonymous SSA
> names before, the introduced condition would necessarily not be met, so it
> effectively reduced the amount of copyrename coalescing without any
> rationale AFAICT.
> 
> Richi, do you happen to have any insight as to why you changed copyrename to
> avoid coalescing rootless ssa names?

Because "coalescing" them doesn't do anything.  copyrename coalescing
assigns the same underlying DECL to SSA names, thus makes SSA_NAME_VAR
the same.  But when both SSA_NAME_VARs are NULL this won't do anything.

So the effect of the patch was rather that there are now way less SSA names
with SSA_NAME_VAR != NULL.  In particular all the pass created ones
(with no relation to user declared variables).  It didn't make sense to
apply copyrename to those (copyrename is to have better debug info in the end,
sth which should be provided by debug stmts today).

>  Reverting it doesn't seem to have any
> ill effects on code correctness (unlike allowing coalescing between rooted
> and rootless vars during out-of-ssa, in gimple_can_coalesce_p, which breaks
> codegen badly, though I haven't dug into why).  I'll attach a patch I'm
> testing to that effect momentarily.

So I'd say the patch can't make a difference - it at most can end up
re-writing the type of an SSA name and then having followup effects
in out-of-SSA coalescing?  For some odd reason we are ignoring some
type differences in copyrename coalescing.

But - how does either root1 or root2 end up non-zero at

  /* Set the root variable of the partition to the better choice, if there is
     one.  */
  if (!ign2 && root2)
    replace_ssa_name_symbol (partition_to_var (map, p3), root2);
  else if (!ign1 && root1) 
    replace_ssa_name_symbol (partition_to_var (map, p3), root1);
  else
    gcc_unreachable ();     

?  That is, the only effect of your patch is to do partiton_union?  Ah,
and then go to:

  /* Now one more pass to make all elements of a partition share the same
     root variable.  */

  for (x = 1; x < num_ssa_names; x++)
    {
      part_var = partition_to_var (map, x);
      if (!part_var)
        continue;
      var = ssa_name (x);
      if (SSA_NAME_VAR (var) == SSA_NAME_VAR (part_var))
        continue;
      if (debug)
        {
          fprintf (debug, "Coalesced ");
          print_generic_expr (debug, var, TDF_SLIM);
          fprintf (debug, " to ");
          print_generic_expr (debug, part_var, TDF_SLIM);
          fprintf (debug, "\n");
        }
      stats.coalesced++;
      replace_ssa_name_symbol (var, SSA_NAME_VAR (part_var));
    }

and wreck SSA names type (and "debug" identifier) only?  That really sounds
bogus.

The correct fix would be to allow the kind of type differences copyrename
coalescing allows also in out-of-SSA coalescing.

That is, I don't see how the patch makes sense.