Re: [PATCH] PR rtl-optimization/106594: Preserve zero_extend in combine when cheap.

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Sun, Mar 05, 2023 at 08:43:20PM +0000, Tamar Christina wrote:
> > On 3/5/23 12:28, Tamar Christina via Gcc-patches wrote:
> > > The regression was reported during stage-1. A patch was provided during
> > stage 1 and the discussions around combine stalled.
> > >
> > > The regression for AArch64 needs to be fixed in GCC 13. The hit is too big just
> > to "take".
> > >
> > > So we need a way forward, even if it's stage-4.
> > Then it needs to be in a way that works within the design constraints of
> > combine.
> > 
> > As Segher has indicated, using a magic constant to say "this is always cheap
> > enough" isn't acceptable.  Furthermore, what this patch changes is combine's
> > internal canonicalization of extensions into shift pairs.
> > 
> > So I think another path forward needs to be found.  I don't see hacking up
> > expand_compound_operation is viable.
> 
> I'm not arguing at all about the merits of the patch. My argument was about Segher saying he doesn't think this is a P1 regression or one that should be addressed in stage-4.

That is not what I said (in the PR).  I said:
  Either this should not be P1, or the proposed patch is taking
  completely the wrong direction.  P1 means there is a regression.
  There is no regression in combine, in fact the proposed patch would
  *cause* regressions on many targets!
(<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106594#c13>)

> We noticed and reported the regression early on during stage-1.  So I'm unsure what else we should have done and it's not right to waive off fixing it now, otherwise what's the point in us filing bug reports.

Something that fixes the regression is of course welcome.  But something
that *causes* many regressions is not.  Something that makes
compound_operation stuff better on all targets is more than welcome as
well, but *better* on *all* targets, not regressing most.  This really
is stage 1 material most likely.  I have been chipping away at this for
years, I don't expect any trivial patch can help, and it certainly won't
solve most problems here.

Maybe a target hook for this is best.  But not a completely ad-hoc one,
something usable and maintainable please.  So, it should say we do not
want certain kinds of code (or what kind of code we want instead), and
it should not add magic to the bowels of basic passes, magic that just
happens to make the code of particular testcases look better on a
particular target.

Yes, *look* better: I have seen no proof or indication that this would
actually generate better code, not even on just aarch, let alone on the
majority of targets.  As I said I have a test running, you may be lucky
even :-)  It has to run for about six hours more and after that it needs
analysis still (a few more hours if it isn't obviously always better or
worse), so expect results tomorrow night at the earliest.


Segher