Re: [PING][PATCH v2] combine: Don't turn (mult (extend x) 2^n) into extract [PR96998]

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

On Fri, Oct 09, 2020 at 09:38:09AM +0100, Alex Coplan wrote:
> Hi Segher,
> 
> On 08/10/2020 15:20, Segher Boessenkool wrote:
> > On Thu, Oct 08, 2020 at 11:21:26AM +0100, Alex Coplan wrote:
> > > Ping. The kernel is still broken on AArch64.
> > 
> > You *cannot* fix a correctness bug with a combine addition.
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2020-September/555158.html
> explains why we do precisely that.

And it still is wrong.

> Also, as your own testing confirmed, the patch does indeed fix the issue.

No, it did not.  It showed that before the patch the bug was hit, and
after it it was not.  It does not show the bug was solved.

> > So please fix the target bug first.
> 
> I think the problem here -- the reason that we're talking past each
> other -- is that there are (at least) two parts of the codebase that can
> be blamed for the ICE here:
> 
> 1. aarch64: "The insn is unrecognized, so it's a backend bug
> (missing pattern or similar)."
> 
> 2. combine: "Combine produces a non-canonical insn, so the backend
> (correctly) doesn't recognise it, and combine is at fault."

That cannot cause ICEs!  *That* is the issue.  It is *normal* for insns
combine comes up with to not be allowed by the backend (or recognised
even); in that case, that instruction combination is simply not made.
The code was valid before, and stays valid.  That is all that combine
does: it may or may not change some instructions to semantically
equivalent instructions that the cost model thinks are better to have.

You *cannot* depend on combine to make *any* particular combination or
simplification or anything.  There are hundreds of reasons to abort a
combination.

3.  The insn is not valid for the target, so the target does *not*
recognise it, and combine does *not* make that combination, *and all is
good*.

That is how it is supposed to work (and how it *does* work).

If the input to combine contains invalid instructions, you have to fix a
bug in some earlier pass (or the backend).  All instructions in the
instruction stream have to be valid (some time shortly after expand, and
passes can do crazy stuff internally, so let's say "between passes").

> Now I initially (naively) took interpretation 1 here and tried to fix
> the ICE by adding a pattern to recognise the sign_extract insn that
> combine is producing here:
> https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553605.html
> 
> Howerver, in the review of that patch, Richard opened my eyes to
> interpretation 2, which in hindsight is clearly a better way to fix the
> issue.
> 
> Combine already does the canonicalisation for the (ashift x n) case, so
> it seems like an obvious improvement to do the same for the (mult x 2^n)
> case, as this is how shifts are represented inside mem rtxes.

An improvement perhaps, but there is still a bug in the backend.  It is
*normal* for combine to create code invalid for the target, including
non-canonical code.  You should not recognise non-canonical code if you
cannot actually handle it correctly.

> Again, please see Richard's comments here:
> https://gcc.gnu.org/pipermail/gcc-patches/2020-September/554518.html
> 
> > 
> > I haven't had time to look at your patch yet, sorry.
> 
> Not to worry. Hopefully this clears up any confusion around what we're
> trying to do here and why.

It doesn't, unfortunately.  But maybe you understand now?


Segher