[PATCH] D38236: [PowerPC] eliminate partially redundant compare instruction

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:571
+    }
+    else if (Inst->isFullCopy())
+      NextReg = Inst->getOperand(1).getReg();
----------------
I'm not sure how this full-copy case would come up, unless:
 1. The source or destination is a physical register (but we bail out in that case)
 2. The source or destination have different register classes
 3. This is run after PHI elimination (which this pass is not)

I can't think of a case where (2) would come up (we don't have different register classes for GPRs, except for r0 restrictions, but I believe there we just end up with one vreg with a union register class).

I short, unless you've seen this in practice, I recommend not handling it at all.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:616
+    // successors to avoid potentially increasing code size.
+    return (BB.succ_size() == 1);
+  };
----------------
Please remove the parentheses here.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:640
+    if (isEligibleBB(*Pred1MBB) && isEligibleForMoveCmp(*Pred2MBB)) {
+    }
+    else if (isEligibleBB(*Pred2MBB) && isEligibleForMoveCmp(*Pred1MBB)) {
----------------
I'm okay with this, but we shouldn't have nothing in the braces. A comment is okay.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:702
+    // As partially redundant case, we additionally handle if MBB2 has one
+    // another predecessor, which has only one successor (MBB2).
+    // In this case, we move the compre instruction originally in MBB2 into
----------------
another -> additional


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:918
+    if (IsPartiallyRedundant) {
+      DEBUG(dbgs() <<"The following compare is moved into BB#" <<
+            MBBtoMoveCmp->getNumber() << " to handle partial redundancy.\n");
----------------
Space after first `<<`.


https://reviews.llvm.org/D38236