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

[Hiroshi Inoue via Phabricator via llvm-commits]

inouehrs marked 4 inline comments as done.
inouehrs added inline comments.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:571
+    }
+    else if (Inst->isFullCopy())
+      NextReg = Inst->getOperand(1).getReg();
----------------
hfinkel wrote:
> 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.
Disabling this full-copy case reduces the number of eliminated compares by about 1% during the bootstrap (23483 -> 23191), although I have not investigated which optimizer generates COPYs.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:616
+    // successors to avoid potentially increasing code size.
+    return (BB.succ_size() == 1);
+  };
----------------
hfinkel wrote:
> Please remove the parentheses here.
I think we cannot eliminate the parentheses of lambda.


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


================
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
----------------
hfinkel wrote:
> another -> additional
Fixed.


================
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");
----------------
hfinkel wrote:
> Space after first `<<`.
Fixed.


https://reviews.llvm.org/D38236