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

[Chandler Carruth]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Looks really close, just need to sort out the offset simplification.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:387-389
@@ -386,1 +386,5 @@
 
+    if (!isa<Constant>(LHS) && !isa<Constant>(RHS))
+      if (!simplifyUsingOffsets(LHS, RHS))
+        return Base::visitBinaryOperator(I);
+
----------------
This doesn't really seem correct to me...

For example, multiplication such as  "(B + X) * (B + Y)" does not simplify to "X * Y". Even addition doesn't simplify that way.

I think it would be more clear (and correct) to explicitly handle the math that simplifies here rather than trying to share a routine. Test for subtraction and that LHS and RHS are in the simplified addresses mapping. If they are, you can write a comment about how the base addresses cancel and the result is the CaonstantExpr difference of the offsets.

I don't think you really need to even think about falling through to the fancy InstSimplify logic here because you only can do anything when you have boring constant offsets.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:456
@@ +455,3 @@
+    if (!isa<Constant>(LHS) && !isa<Constant>(RHS))
+      if (!simplifyUsingOffsets(LHS, RHS))
+        return Base::visitCmpInst(I);
----------------
If you take my advice above, I would also inline the logic here. I'm not sure there is really going to be that much shared between the two when you're done. You can really *only* handle subtraction above, but here you can handle any comparison and really want to just fall back on the same logic.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:597
@@ -528,2 +596,3 @@
+
       // Add BB's successors to the worklist.
       for (BasicBlock *Succ : successors(BB))
----------------
I would leave a hint in this comment that this is the fallback if we can't directly fold the successor above.


http://reviews.llvm.org/D10206