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

Sun Jan 9 23:44:20 PST 2022

benshi001 added a comment.

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116468