[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, my comments are inline.

I am also curious: have you run test-suite <https://www.llvm.org/docs/TestSuiteGuide.html> and make-check-llvm <https://www.llvm.org/docs/TestingGuide.html#id10> on this code? For `test-suite` I am curious about the `SingleSource`, `MultiSource`, and `CTMark` directories. The first can help identify miscompiles or compiler crashes while last one can help identify compile-time performance regressions.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:374
 
-  bool EverChanged = false;
+  bool EverChanged = processBBWithCommonDomCondition(F);
+  DTU->getDomTree();
----------------
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.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:376
+  DTU->getDomTree();
+  for (auto &BB : F)
+    if (!Unreachable.count(&BB) && !DT.isReachableFromEntry(&BB))
----------------
This is expensive for large block counts of `F`. It would be better to update `Unreachable` as you alter the state of BBs within the new logic.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:567
+}
+
+/// collectReachableBBSet - Create a set of reachable basic blocks from BB in
----------------
I think you can get the `Level` from `DT` using the `getLevel()` method without generating it recursively yourself.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:595
+/// Both SuccBB0 and SuccBB1 are the successors of BBa.
+static unsigned findPredSourceEdge(BasicBlock *BBa, BasicBlock *BB,
+                                   BasicBlock *Pred, BasicBlock *SuccBB0,
----------------
This should probably return an enum similar to `enum class UpdateStrategy : unsigned char { Eager = 0, Lazy = 1 };` Magic numbers make maintainability more difficult.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:697
+bool JumpThreadingPass::processBBWithCommonDomCondition(Function &F) {
+  DominatorTree DT(F);
+  DenseMap<BasicBlock*, unsigned> BBtoDepth;
----------------
Why instantiate another instance of `DT`? The location where you call this code has `DTU` which you can pass in. For large `F` generating the DT is expensive.  It's also a unique instance of `DT` that knows nothing about `DTU->DT`.


================
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();
----------------
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.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:730
+          if (Preds0.size() == 0) {
+            conditionalToUnconditional(DTU, BB, 1, 0);
+            UB++;
----------------
This code makes me nervous. On line 727 you call `findBBWithCommonDomCondition(DT, ...` but here you call `conditionalToUnconditional(DTU, ...`. The `JumpThreadingPass` creates `DTU` with `UpdateStrategy::Lazy`. This means the calls to `DTU->deleteEdgeRelaxed(...)` in `conditionalToUnconditional()` and the calls to `DT.dominates()` in `findBBWithCommonDomCondition()` may lead to an inconsistent state.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:744
+              {
+                // TODO: It is necessary to explain the validity of
+                // temporarily increasing BBDupThreshold.
----------------
Please explain the reasoning behind increasing `BBDupThreshold`


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

https://reviews.llvm.org/D57953