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

[Masaki Arai via Phabricator via llvm-commits]

masakiarai marked 7 inline comments as done.
masakiarai added inline comments.


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



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


================
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,
----------------
brzycki wrote:
> This should probably return an enum similar to `enum class UpdateStrategy : unsigned char { Eager = 0, Lazy = 1 };` Magic numbers make maintainability more difficult.
You are right.
I will fix this.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:697
+bool JumpThreadingPass::processBBWithCommonDomCondition(Function &F) {
+  DominatorTree DT(F);
+  DenseMap<BasicBlock*, unsigned> BBtoDepth;
----------------
brzycki wrote:
> 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`.
The correct patch uses only DTU.


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



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:730
+          if (Preds0.size() == 0) {
+            conditionalToUnconditional(DTU, BB, 1, 0);
+            UB++;
----------------
brzycki wrote:
> 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.
I understand your concerns.
The correct patch uses only DTU.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:744
+              {
+                // TODO: It is necessary to explain the validity of
+                // temporarily increasing BBDupThreshold.
----------------
brzycki wrote:
> Please explain the reasoning behind increasing `BBDupThreshold`
It is unnecessary to change BBDupThreshold here.
I have confirmed that increasing BBDupThreshold here increases the opportunity for some optimization about HPC applications.
However, to generate CFG like GCC(that is my immediate goal), I do not need to change the value.



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

https://reviews.llvm.org/D57953