[PATCH] D15508: [JumpThreading] Split select that has constant conditions coming from the PHI node

[Chad Rosier via llvm-commits]

mcrosier added inline comments.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1903
@@ +1902,3 @@
+/// select if the associated PHI has at least one constant.  If the unfolded
+/// select is not jump-threaded, it can be folded again in the later
+/// optimizations.
----------------
can -> will

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1922
@@ +1921,3 @@
+/// TryToUnfoldSelectInCurrBB
+SelectInst *JumpThreading::ShouldUnfoldSelect(BasicBlock *BB) {
+  // Only searches BB that can be threaded
----------------
Honestly, I would get rid of this helper function and inline it into TryToUnfoldSelectInCurrBB().  It's highly unlikely this function will be reused and the current TryToUnfoldSelectInCurrBB implementation is trivially small..

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1923
@@ +1922,3 @@
+SelectInst *JumpThreading::ShouldUnfoldSelect(BasicBlock *BB) {
+  // Only searches BB that can be threaded
+  if (LoopHeaders.count(BB))
----------------
Please add a more verbose comment.  Perhaps,

  // If threading this would thread across a loop header, don't thread the edge.
  // See the comments above FindLoopHeaders for justifications and caveats.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1927
@@ +1926,3 @@
+
+  // Look for the Phi/Select pair in the same basic block.
+  // PHI is the condition of the Select.
----------------
Please maximize 80-column comments (i.e., only wrap the line after 80 chars).

Perhaps,

  // Look for a Phi/Select pair in the same basic block.  The Phi feeds the
  // condition of the Select and at least one the incoming values is a constant.

================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1949
@@ +1948,3 @@
+      if (isa<ConstantInt>(PN->getIncomingValue(i)))
+        HasConst = true;
+    }
----------------
Can't you remove the 'HasConst' bool entirely by returning SI here?  This will also cause the loop to early exit.


Repository:
  rL LLVM

http://reviews.llvm.org/D15508