[PATCH] D116468: [AArch64] Combine ADD/SUB instructions when they contain a 24-bit immediate.

Mon Jan 10 20:01:53 PST 2022

benshi001 added a comment.

In D116468#3233108 <https://reviews.llvm.org/D116468#3233108>, @red1bluelost wrote:

> In D116468#3230617 <https://reviews.llvm.org/D116468#3230617>, @benshi001 wrote:
>
>> In D116468#3228702 <https://reviews.llvm.org/D116468#3228702>, @red1bluelost wrote:
>>
>>> In D116468#3224907 <https://reviews.llvm.org/D116468#3224907>, @dmgreen wrote:
>>>
>>>> In D116468#3224507 <https://reviews.llvm.org/D116468#3224507>, @benshi001 wrote:
>>>>
>>>>> I am quite sorry for that. Though `make check-all` passed and everything was good on amd64, the clang built into aarch64 ELF and running on aarch64-linux crashed. And I have no aarch64 host machine to debug that (although I guess it should be a minor bug).
>>>>
>>>> Oh I see. Sorry for not seeing that message. I hadn't realized it was reverted.
>>>>
>>>> I have no strong opinion whether this is best done as an AArch64MIPeephole or in MachineCombine. They are probably both fine for this kind of thing. Whichever @red1bluelost thinks will be easiest to extend to SUBS instructions that write nzcv sounds OK to me, if the goal here is to address https://github.com/llvm/llvm-project/issues/51482.
>>>
>>> Either method (Peephole or MachineCombine) would be easy to extend for the SUBS instructions. I think the Peephole approach would be better, it seems more encapsulated than my approach.
>>>
>>> What is the status of D111034 <https://reviews.llvm.org/D111034> @benshi001, is it likely to be committed or is that bug still holding it up? The buildbot logs don't appear anymore so I can't see where that bug happened. Could that bug also be present with my MachineCombine patch?
>>
>> @red1bluelost, you are appreciated to continue this optimization along with the peephole approach, if you are sure it no longer crashes on aarch64-linux host. As I have explained, I have no aarch64-linux host and can not gurantee that. Thanks! I am glad if anybody can add this optmization to the main branch ASAP.
>
> Ok. I will try to see if I can find the bug with the Peephole if possible. I have been testing it this weekend through the Linaro Docker container mentioned in D111034 <https://reviews.llvm.org/D111034> but using QEMU emulation of AArch64. It seems like it can replicate the aarch64-linux host environment but is just really slow. (I don't have an aarch64 host either).
>
> I'll also see about finishing up the MachineCombine approach and testing it too, incase it would be easier to just finish this approach.
>
> If I manage to fix the Peephole approach, should I edit D111034 <https://reviews.llvm.org/D111034>? Or should I start a new patch review for the Peephole?

I suggest you start a new patch and mention D111034 <https://reviews.llvm.org/D111034> :)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116468/new/

https://reviews.llvm.org/D116468