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

Mon Jan 10 17:37:10 PST 2022

red1bluelost added a comment.

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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116468