[PATCH] D22779: Clone block with icmp+branch if it likely results in further jump threading

[Sebastian Pop via llvm-commits]

sebpop added a comment.

I like the idea that this patch is trying to address.

In GCC one can jump-thread across several BBs, whereas this patch "hard-codes" the pattern-matching to only 2 BBs.
It would be good to extend this patch to add a flag that selects the max path length to be jump-threaded.
Then we would need some performance analysis to select a good default for that flag.

The idea is to perform the analysis backwards: one starts from a condition and walk back on the SSA to the definitions involved in that condition.
If on one of the paths the condition can be statically evaluated, then that path is duplicated, the condition is removed, and the final jump is to the destination evaluated at compile time.

This algorithm in turn can be extended to jump-thread across back-edges in loops: that gives about 30% speedup on Coremark, and of course that pattern is present in parsers that implement Finite State Machines (FSM) a loop containing a switch statement, and each case sets the value for the next switch condition in the next iteration.  One other example hit by GCC's jump-threading is in libART.

The cost function for code duplication has to be adjusted to account for the full length of the path to be duplicated:
the code in DuplicateBBIntoPreds() evaluates the size of only one BB to be duplicated based on this flag:

  static cl::opt<unsigned>
  BBDuplicateThreshold("jump-threading-threshold",
          cl::desc("Max block size to duplicate for jump threading"),
          cl::init(6), cl::Hidden);

Now that we copy two BBs their length will be evaluated separately.
I think we need another flag that evaluates the length of the full path to be duplicated.



================
Comment at: include/llvm/Transforms/Scalar/JumpThreading.h:112
                   BasicBlock *SuccBB);
-  bool DuplicateCondBranchOnPHIIntoPred(
-      BasicBlock *BB, const SmallVectorImpl<BasicBlock *> &PredBBs);
+  bool DuplicateBBIntoPreds(BasicBlock *BB,
+                            const SmallVectorImpl<BasicBlock *> &PredBBs);
----------------
I see that the other functions in this file do not follow the convention:
now that you are changing the name of this function, please make it start with lower-case.



https://reviews.llvm.org/D22779