[PATCH] D25477: [JumpThreading] Unfold selects that depend on the same condition

[Sebastian Pop via llvm-commits]

sebpop added a comment.

Please fix the few formating issues.
The patch looks good otherwise.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2030
+    // constant.
+    PHINode *PN; SelectInst* SI;
+    if ((PN = dyn_cast<PHINode>(&I)) && (SI = GetSelectFedByPhi(PN))) {
----------------
Please run clang-format on your patch.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2033
+        ExpandSelect(SI);
+        changed = true;
+    }
----------------
Please add a continue statement here: there is nothing else executed in the loop.
This will make it easier to read the rest of the loop body.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2038
+    // several selects that use the same boolean, they are candidates for jump
+    // threading and therefore we should unfold them.
+    else if (I.getType()->isIntegerTy(1)) {
----------------
Let's move this comment close to the loop that looks for selects using the same bool... down...


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2039
+    // threading and therefore we should unfold them.
+    else if (I.getType()->isIntegerTy(1)) {
 
----------------
Also remove the "else" after adding the "continue" above.

You could even flip the if condition:

  if (!I.getType()->isIntegerTy(1))
    continue;

making all the rest of the loop one less indentation level.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2041
 
-    bool HasConst = false;
-    for (unsigned i = 0; i != NumPHIValues; ++i) {
-      if (PN->getIncomingBlock(i) == BB)
-        return false;
-      if (isa<ConstantInt>(PN->getIncomingValue(i)))
-        HasConst = true;
-    }
+      SmallVector<SelectInst*, 4> Selects;
+      for (Value *U : I.users())
----------------
... down here.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2047
+        continue;
+      else
+        changed = true;
----------------
No need for else after continue.


https://reviews.llvm.org/D25477