[PATCH] D55264: [Jump Threading] Unfold a select instruction that feeds a switch statement via a phi node

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Hi @amehsan , I think the patch looks mostly good but for a few small comments or variable names to enhance readability.



================
Comment at: include/llvm/Transforms/Scalar/JumpThreading.h:146
   bool TryToUnfoldSelect(CmpInst *CondCmp, BasicBlock *BB);
+  bool TryToUnfoldSelect(SwitchInst *, BasicBlock *BB);
   bool TryToUnfoldSelectInCurrBB(BasicBlock *BB);
----------------
This should probably be written as:
```
bool TryToUnfoldSelect(SwitchInst *SI, BasicBlock *BB);
```


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1176
+  if (SwI != nullptr)
+    TryToUnfoldSelect(SwI, BB);
+
----------------
Style again:

```
if (SwitchInst *SI = dyn_cast<SwitchInst>(BB->getTerminator()))
  TryToUnfoldSelect(SI, BB);
```


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2394
 }
 
+void JumpThreadingPass::UnfoldSelectInstr(BasicBlock *Pred, BasicBlock *BB,
----------------
Please add documentation here describing the requirements for the code shape checks before entering `UnfoldSelectInstr`. For example, it's not easy to discern Pred has an unconditional branch to BB without looking at the call sites and the function.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2431
+bool JumpThreadingPass::TryToUnfoldSelect(SwitchInst *SwI, BasicBlock *BB) {
+  PHINode *SwVar = dyn_cast<PHINode>(SwI->getCondition());
+
----------------
Possibly `CondPHI` instead of `SwVar`? 


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2438
+    BasicBlock *Pred = SwVar->getIncomingBlock(I);
+    SelectInst *SI = dyn_cast<SelectInst>(SwVar->getIncomingValue(I));
+
----------------
`SI` is a bit confusing to me. I think of it as the top-level select. This might be better named as `PredSI`.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:2442
+    // predecessor.
+    if (!SI || SI->getParent() != Pred || !SI->hasOneUse())
+      continue;
----------------
Could you add a little more to the comment discussing why we don't want to alter the select for the 2nd and 3rd check here?


================
Comment at: test/Transforms/JumpThreading/select.ll:437
+; CHECK: switch i32 [[REG2]]
+}
----------------
Is this check sufficient? There is also a change in `land.lhs.true` where we no longer contain the `%spec.select` statement and the terminator instruction changes from unconditional to conditional.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55264/new/

https://reviews.llvm.org/D55264