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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:571
+    }
+    else if (Inst->isFullCopy())
+      NextReg = Inst->getOperand(1).getReg();
----------------
inouehrs wrote:
> 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.
Okay.


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

  return BB.succ_size() == 1;


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:640
+    if (isEligibleBB(*Pred1MBB) && isEligibleForMoveCmp(*Pred2MBB)) {
+      // We assume Pred1MBB is the BB containing compare to be merged and
+      // Pred2MBB is the BB we will append a compare instruction.
----------------
containing compare -> containing the compare


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:641
+      // We assume Pred1MBB is the BB containing compare to be merged and
+      // Pred2MBB is the BB we will append a compare instruction.
+      // Hence we can proceed as is.
----------------
the BB we will -> the BB to which we will


https://reviews.llvm.org/D38236