[PATCH] D37211: [PowerPC] eliminate redundant compare instruction

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:436
+        BII != BB.instr_end() &&
+        (*BII).getOpcode() == PPC::BCC &&
+        (*BII).getOperand(1).isReg()) {
----------------
I think you can also handle BCCLR and BCA. BCA doesn't come up much, but we can have BCCLR in loops where this might be helpful.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:465
+//
+// This method merges two compare instructions in two MBBs and modify
+// compare and conditional branch instructions if needed.
----------------
modify -> modifies the


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:527
+      unsigned Cmp2Operand2 = CMPI2->getOperand(2).getReg();
+      if (Cmp1Operand1 == Cmp2Operand1 && Cmp1Operand2 == Cmp2Operand2) {
+        // Same pair of registers in the same order; ready to merge as is.
----------------
I think that you can run into trouble if these are physical registers (similar to the problem recently uncovered with PPCBranchCoalescing. This will work for virtual registers, but not for physical registers, unless MRI->isConstantPhysReg or TRI->isCallerPreservedPhysReg is true, or you scan the instructions in the first BB to make sure that nothing changes the register.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:537
+        switch (PredCond) {
+        case PPC::PRED_LT:
+          NewPredicate2 = (unsigned)PPC::getPredicate(PPC::PRED_GT, PredHint);
----------------
I don't think you need this switch, but can use:

  NewPredicate2 = (unsigned)PPC::getSwappedPredicate(Pred);


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:558
+      // the operand register must be same for two compare instructions.
+      if (CMPI1->getOperand(1).getReg() != CMPI2->getOperand(1).getReg())
+        continue;
----------------
See comment above re: if these are physical registers.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:571
+
+        unsigned predToInc1 = getPredicateToIncImm(BI1, CMPI1);
+        unsigned predToDec1 = getPredicateToDecImm(BI1, CMPI1);
----------------
Local variables should start with a capital letter.


https://reviews.llvm.org/D37211