On Mon, Oct 12, 2020 at 05:19:58PM +0100, Richard Sandiford wrote: > Segher Boessenkool <seg...@kernel.crashing.org> writes: > > 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. > > 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. Segher
