[PATCH] D57953: [Jump Threading] Convert conditional branches into unconditional branches using GVN results

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Hello @masakiarai, the new patch looks better, thank you. My updated comments are inline.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:374
 
-  bool EverChanged = false;
+  bool EverChanged = processBBWithCommonDomCondition(F);
+  DTU->getDomTree();
----------------
masakiarai wrote:
> brzycki wrote:
> > Running `processBBWithCommonDomCondition` at this location means the code can only ever catch this pattern once when the initial IR passed to JumpThreading contains it. If the pattern ever emerges in the IR from changes within the `do { ... } while();` loop below it will not be detected.
> It is possible to place processBBWithCommonDomCondition call in the loop, but in reality, there was little effect.
> In theory, I can create situations that are effective.
> JumpThreading is executed multiple times, but since the first time is before GVN, the execution of processBBWithCommonDomCondition at that time has no meaning.
> 
Understood. I'm still thinking there are cases your testing didn't encounter that emerge from other JT optimizations. I suppose real-world usage will discover them, if at all.

Also, I'd really prefer if `processBBWithCommonDomCondition()` marked BBs unreachable as it changed `F` instead of coming back and doing yet-another loop over all the BBs. For large `F` these multiple iterations over all the blocks adds non-trivial deltas of compile-time.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:703
+  MapVector<Value *, SmallVector<BasicBlock *, 4>> CtoBBList;
+  for (Function::iterator BB = F.begin(), BE = F.end(); BB != BE; ++BB) {
+    Instruction *Terminator = BB->getTerminator();
----------------
masakiarai wrote:
> brzycki wrote:
> > This feels like the code here is more of a function-level-pass-within-a-pass than an extension of JumpThreading. I have concerns the number of loops and recursive calls over the `BB`s of `F` will add to the overall compile time.
> I do not think that I should create a new pass in the following reasons:
> (1) The essential effect of this optimization is about branches.
> (2) The most complicated process of my modification depends on JumpThreadingPass::ThreadEdge.
> 
Understood. I made the comment because most of the optimizations in JT rely on analysis at the `BB` level but this new enhancement works at the `F` level. It's a bit of a departure for JT and I'm not sure if the community as a whole is interested in going this direction. I don't think I alone can make that decision...


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:680
+  CondBr->eraseFromParent();
+  DTU->deleteEdgeRelaxed(BBd, ToRemoveSucc);
+}
----------------
Does this need to be `deleteEdgeRelaxed()`? If at all possible please don't use this variant of the `DTU` method. There's actually work underway to try and deprecate/remove it. (See D58170 and D57881 if you're interested)


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:720
+          else if (Preds0.size() == 0) {
+            conditionalToUnconditional(DTU, BB, 1, 0);
+            UB++;
----------------
Please no magic numbers.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:724
+          } else if (Preds1.size() == 0) {
+            conditionalToUnconditional(DTU, BB, 0, 1);
+            UB++;
----------------
Please no magic numbers.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:729
+            BranchInst *CondBr = dyn_cast<BranchInst>(BB->getTerminator());
+            BasicBlock *SuccBB0 = CondBr->getSuccessor(0);
+            BasicBlock *SuccBB1 = CondBr->getSuccessor(1);
----------------
This function is fairly complex. I know I'd appreciate a few more comments describing the logic and why certain checks are happening.


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

https://reviews.llvm.org/D57953