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 12/10/2020 12:14, Segher Boessenkool wrote:
> On Mon, Oct 12, 2020 at 05:19:58PM +0100, Richard Sandiford wrote:
> > 
> > I agree there's a target bug here.  Please see the explanation I posted
> > in: https://gcc.gnu.org/pipermail/gcc-patches/2020-September/554518.html
> > (especially the first sentence quoted below :-)).
> > 
> >     The situation as things stand is that aarch64 has a bug: it accepts
> >     an odd sign_extract representation of addresses, but doesn't accept
> >     that same odd form of address as an LEA.  We have two options:
> > 
> >     (a) add back instructions that recognise the odd form of LEA, or
> >     (b) remove the code that accepts the odd addresses
> > 
> >     I think (b) is the way to go here.
> 
> Either seems to be fine.
> 
> >     But doing that on its own
> >     would regress code quality.  The reason we recognised the odd
> >     addresses in the first place was because that was the rtl that
> >     combine happened to generate for an important case.
> > 
> > So if we go for (b) but fix the aarch64 bug strictly before the
> > combine patch, we would need to:
> 
> This is necessary to be able to evaluate what such a combine patch
> does in practice -- so there is no other way.
> 
> > (1) Apply the target fix and adjust the testsuite markup to make sure
> >     that the git commit doesn't regress anyone's test results.
> 
> It is normal to regress the testsuite for a little while.
> 
> > (2) Apply the combine patch and revert the testsuite markup changes
> >     from (1).
> > 
> > That seems like make-work, and would still show as a blip for
> > people doing performance tracking.
> 
> Yes, that is make-work.  Just regress the testsuite.
> 
> You do not even have to apply the target patch first (but you need to
> send it as separate patch, so that other people can test it!)
> 
> > If you prefer, we could fix the aarch64 bug and patch combine as a
> > single commit.  See:
> > 
> > https://gcc.gnu.org/pipermail/gcc-patches/2020-September/554257.html
> > 
> > for the full patch, including the aarch64 bugfix.
> 
> I need separate patches, so that I can see what the current combine
> does, without ICEing all over.  That is all.  Send it as a series of two
> patches, or something.

Apologies for the misunderstanding and my inaccurate summary in the previous
email, there is of course a target bug here as well.

I've sent the patches as a two-patch series where the first patch fixes the
AArch64 bug and the second restores code quality with the patch to combine:
https://gcc.gnu.org/pipermail/gcc-patches/2020-October/556237.html
https://gcc.gnu.org/pipermail/gcc-patches/2020-October/556238.html

Thanks,
Alex