[PATCH] D17942: [AArch64] Optimize test and branch sequence when the test's constant operand is power of 2

[Balaram Makam via llvm-commits]

bmakam marked 5 inline comments as done.
bmakam added a comment.

Thanks for the comments Chad and Tim,
I will post an updated patch including all your suggestions.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3016
@@ +3015,3 @@
+  // Look for AND if MI is CBZ/CBNZ and DefMI is a COPY instruction
+  if (!IsTestAndBranch && DefMI->isCopy()) {
+    if (DefMI->getParent() != MBB) return false;
----------------
t.p.northover wrote:
> This looks overly specialized to me: "look through precisely one COPY, but don't accept a direct AND".
> 
> Shouldn't we make the look-through-copy logic generic (so it applies to the CSINC/CBZ too), and iterative?
Thanks Tim, this looks reasonable to me. I will post an updated patch including this suggestion.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3052
@@ +3051,3 @@
+
+    if (Imm > 31) {
+      MRI->constrainRegClass(NewReg, &AArch64::GPR64RegClass);
----------------
mcrosier wrote:
> Should you be checking the Imm value here?  Isn't it possible to have an Imm <= 31, but still be defining a 64bit register?
> 
> I'm wondering if the condition should be something more like (AndDefMI->getOpcode() == AArch64::ANDXri), as is done above?
The immediate is the bit that is tested whether it is set or not, so if it is less than 32 and x[0-9]+, #c is same as w[0-9]+, #c. Furthermore the integrated linker treats and x[0-9]+, #c differently and transforms it into and x[0-9]+, 32+#c if c <32. I think this is a bug in the integrated linker, if we use no-integrated-linker and x[0-9]+, #c is transformed into and w[0-9]+, #c automatically. I therefore am checking the immeadiate value to avoid dependency on the underlying linker.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3073
@@ -3006,3 +3072,3 @@
   // Look for CSINC
   if (!(DefMI->getOpcode() == AArch64::CSINCWr &&
         DefMI->getOperand(1).getReg() == AArch64::WZR &&
----------------
t.p.northover wrote:
> I'd be inclined to make this whole thing a switch on `DefMI->getOpcode()` once it's been found. They're handled more like sibling cases anyway, and I could easily see us adding other patterns later.
Sounds reasonable to me again, will do in updated patch. Just out of curiosity, what other patterns do you think can be added?


http://reviews.llvm.org/D17942