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

[Sebastian Pop via Phabricator via llvm-commits]

sebpop added a comment.

This seems to be a useful transform that is not yet covered by the current implementation of jump-threading.
I think GCC calls it control dependence DCE.
Please run and report performance numbers on the testsuite and other benchmarks that you have access to.



================
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:
> 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...
I am also inclined to say that this does not belong to jump-threading: the analysis relies on gvn to have executed and replaced all equivalent `%cmp` values.  Moving this code to a new pass would allow more flexibility to schedule the pass after gvn.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:374
 
-  bool EverChanged = false;
+  bool EverChanged = processBBWithCommonDomCondition(F, Unreachable);
+  DTU->getDomTree();
----------------
You are missing a flag to enable/disable this transform.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:599
+    return PredFromBothSuccBB0AndSuccBB1;
+  else if (ReachSuccBB0)
+    return PredFromSuccBB0;
----------------
Please remove the `else` after an if-then that returns.  Here and everywhere else in your patch.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
Please review all the coding rules and apply to your patch.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:718
+          if (Preds0.size() == 0 && Preds1.size() == 0)
+            ;
+          else if (Preds0.size() == 0) {
----------------
Please use `continue;` that tells the reader that there are no more statements to be executed in this loop body.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:719
+            ;
+          else if (Preds0.size() == 0) {
+            conditionalToUnconditional(DTU, BB, 1, 0);
----------------
no `else` needed after a `continue;`


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:722
+            UB++;
+            Changed = true;
+          } else if (Preds1.size() == 0) {
----------------
same here: `continue` and remove `else`


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:724
+          } else if (Preds1.size() == 0) {
+            conditionalToUnconditional(DTU, BB, 0, 1);
+            UB++;
----------------
brzycki wrote:
> Please no magic numbers.
You could replace the arguments of `conditionalToUnconditional(..., 0, 1)` with a single param `enum {KeepThen, KeepElse};`



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:732
+            if (SuccBB0 != SuccBB1 && SuccBB0 != BB && SuccBB1 != BB) {
+              if (ThreadEdge(BB, Preds1, SuccBB1)) {
+                SuccBB1->removePredecessor(BB);
----------------
As the code generation relies on `threadEdge`, you could factor it out from this file and make it usable from other passes.



================
Comment at: test/Transforms/JumpThreading/gvn-dom-cb.ll:16
+; CHECK: br i1 %cmp,
+; CHECK-NOT: br i1 %cmp,
+entry:
----------------
Please add a comment that explains what this test is checking for.  Here and for all the CHECK/CHECK-NOT below.

For this particular case the comment could be:
"Check that jump-threading is able to infer the value of %cmp used at the end of basic block if.end and thus that the IR after jump-threading contains only one conditional branch based on %cmp in basic block entry."

Maybe you can also `CHECK-NOT: label if.else4` and mention in the comment that the basic block becomes unreachable and is eliminated.


================
Comment at: test/Transforms/JumpThreading/gvn-dom-cb.ll:58
+; CHECK: br i1 %cmp,
+; CHECK: br i1 %cmp,
+entry:
----------------
You could mention in the comment that explains what this testcase checks for that "This testcase only differs from `check1` by one annotation `noduplicate` on the call to `show3`, this prevents jump-threading because ... such and such."



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

https://reviews.llvm.org/D57953