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

[Chad Rosier via llvm-commits]

mcrosier added a comment.

Please run clang-format on you patch, Balaram.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2956
@@ -2955,1 +2955,3 @@
 ///
+/// \brief Replace test and branch sequence by TBZ/TBNZ instruction when
+/// the test's constant operand is power of 2.
----------------
s/test/compare/

I. e., Replace compare and branch sequence by..?

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3015
@@ -3005,1 +3014,3 @@
 
+  // Look for AND if MI is CBZ/CBNZ and DefMI is a COPY instruction
+  if (!IsTestAndBranch && DefMI->isCopy()) {
----------------
Please add a period.  It might also be helpful to comment your converting this sequence to tbz/tbnz when the AND is a constant power of 2.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3029
@@ +3028,3 @@
+    if (!AndDefMI) return false;
+    if (!(AndDefMI->getOpcode() == AArch64::ANDWri) &&
+        !(AndDefMI->getOpcode() == AArch64::ANDXri))
----------------
Might be easier to read as: Opc != ANDW && Opc != ANDX.  This should also remove the unneeded parens.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3052
@@ +3051,3 @@
+
+    if (Imm > 31) {
+      MRI->constrainRegClass(NewReg, &AArch64::GPR64RegClass);
----------------
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?

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3054
@@ +3053,3 @@
+      MRI->constrainRegClass(NewReg, &AArch64::GPR64RegClass);
+      if (IsNegativeBranch) {
+        Opc = AArch64::TBNZX;
----------------
Single statement; No need for curly brackets.

You could also use a select statement.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3061
@@ +3060,3 @@
+      MRI->constrainRegClass(NewReg, &AArch64::GPR32RegClass);
+      if (IsNegativeBranch) {
+        Opc = AArch64::TBNZW;
----------------
Same.


http://reviews.llvm.org/D17942