[PATCH] D14332: [ARM] Combine CMOV into BFI where possible

[Chad Rosier via llvm-commits]

mcrosier added inline comments.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10436
@@ +10435,3 @@
+
+  if (CmpZ->getOpcode() != ARMISD::CMPZ)
+    return SDValue();
----------------
I believe the logic in PerformCMOVCombine guarantees this will never be true.  Perhaps we could change this to an assert?

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10447
@@ +10446,3 @@
+  // Canonicalize so that the OR is on the left.
+  if (Op1->getOpcode() == ISD::OR && Op0->getOpcode() != ISD::OR)
+    std::swap(Op0, Op1);
----------------
Is the Op0->getOpcode() != ISD::OR alone not sufficient?

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10471
@@ +10470,3 @@
+  if ((OrCI & KnownZero) != OrCI) {
+    dbgs() << "JM: unknown bits\n";
+    return SDValue();
----------------
This should be guarded by a DEBUG macro.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10487
@@ +10486,3 @@
+  
+  for (unsigned BitInY = 0; BitInY < OrCI.getActiveBits(); ++BitInY) {
+    if (OrCI[BitInY] == 0)
----------------
Assuming the call to OrCI.getActiveBits() doesn't change during the lifetime of the loop, this should only be called once to init an end value..

for (unsigned BitInY = 0, E = OrCI.getActiveBits(); BitInY < E; ++BitInY) {

================
Comment at: test/CodeGen/ARM/bfi.ll:88
@@ +87,1 @@
+}
\ No newline at end of file

----------------
Please add a newline.


Repository:
  rL LLVM

http://reviews.llvm.org/D14332