[PATCH] D71701: [AArch64] Peephole optimization. Merge AND and TST instructions

Tue Jan 28 23:02:09 PST 2020

kpdev42 added a comment.

In D71701#1844832 <https://reviews.llvm.org/D71701#1844832>, @SjoerdMeijer wrote:

> I don't mind taking a look.
>
> First question about where this should live. With `AArch64InstrInfo::mergeInstructions(MachineFunction& MF)` you're iterating of all blocks/instructions, which is typically not something that belongs in `AArch64InstrInfo` because that works on instructions. The generic peephole class does exactly this, which makes calls to e.g. `optimizeCondBranch`. As Eli suggested in the email thread on llvm dev, can you implement and do this in `AArch64InstrInfo::optimizeCondBranch(MachineInstr &MI)`?

It is actually because I was confused that `AArch64InstrInfo::optimizeCondBranch(MachineInstr &MI)` takes only one instruction as parameter, but I need to iterate over BB to find necessary ANDS instruction. Of course I might get BB with `getParent()` of MI and iterate over it. But I thought that such interface would be kind of misleading (I mean - if function takes one instruction as a parameter, but works with BB, it should better takes BB as a parameter (or even function)). Seems to be that I am overthinking a bit here :(

I will change interface from `AArch64InstrInfo::mergeInstructions(MachineFunction& MF)` to `AArch64InstrInfo::mergeAnds(MachineInstr &MI)` asap to proceed with review

In D71701#1845059 <https://reviews.llvm.org/D71701#1845059>, @dmgreen wrote:

> The other option, which might turn out to be simpler is to try and do this in ISel instead. There is a getNodeIfExists(..), which can be used to check if we have an ISD::AND, and an equivalent AArch64ISD::ANDS. Maybe done in performANDCombine?
>
> This is kind of like doing CSE between two different nodes, but ISel is pretty good at that.

It is an interesting suggestion, thank you. At first glance `AArch64TargetLowering` looks like more suitable place for this optimization, but I might be wrong.

Should I implement both approaches (in `AArch64InstrInfo` and in `AArch64TargetLowering`) to compare?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71701