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

[Alex Coplan <alex.coplan@arm.com>]

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.

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

> 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."

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.

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.

Thanks,
Alex