[PATCH] D10206: Handle resolvable branches in complete loop unroll heuristic.

[Chandler Carruth]

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

This looks fantastic. Nit-picky indenting and layout below. Feel free to commit with that addressed.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:467-475
@@ +466,11 @@
+      auto SimplifiedRHS = SimplifiedAddresses.find(RHS);
+      if (SimplifiedLHS == SimplifiedAddresses.end() ||
+          SimplifiedRHS == SimplifiedAddresses.end())
+        return Base::visitCmpInst(I);
+      SimplifiedAddress LHSAddr = SimplifiedLHS->second;
+      SimplifiedAddress RHSAddr = SimplifiedRHS->second;
+      if (LHSAddr.Base != RHSAddr.Base)
+        return Base::visitCmpInst(I);
+      LHS = LHSAddr.Offset;
+      RHS = RHSAddr.Offset;
+    }
----------------
I actually think its better to nest these. Because we're using fall-through to "continue trying", the early-exit is hard to spot here. It also will save some map lookups:

  auto SimplifiedLHS = SimplifiedAddresses.find(LHS)
  if (SimplifiedLHS != SimplifiedAddresses.end()) {
    auto SimplifiedRHS = SimplifiedAddresses.find(RHS);
    if (SimplifiedRHS != SimplifiedAddresses.end()) {
      SimplifiedAddress &LHSAddr = SimplifiedLHS->second;
      SimplifiedAddress &RHSAddr = SimplifiedRHS->second;
      if (LHSAddr.Base == RHSAddr.Base) {
        LHS = LHSAddr.Offset;
        ...

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:478-480
@@ +477,5 @@
+
+    if (Constant *CLHS = dyn_cast<Constant>(LHS)) {
+      if (Constant *CRHS = dyn_cast<Constant>(RHS))
+        if (Constant *C = ConstantExpr::getCompare(I.getPredicate(), CLHS, CRHS)) {
+          SimplifiedValues[&I] = C;
----------------
If you're going to skip the middle {}s (which I'm fine with) skip the outer ones as well.


http://reviews.llvm.org/D10206