[PATCH] D70247: [JumpThreading] Thread jumps through two basic blocks

[Kazu Hirata via Phabricator via llvm-commits]

kazu marked 2 inline comments as done.
kazu added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:1611-1612
+    // predecessors to increase jump threading opportunities.
+    if (MaybeCloneThreadableBasicBlock(BB, Cond))
+      return true;
     return false;
----------------
wmi wrote:
> MaybeCloneThreadableBasicBlock may clone the predecessor of BB, return true and leaves the real threading job to the next call of ProcessBlock(BB). Is it possible that predecessor of BB is cloned but somehow threading doesn't happen in the next call of ProcessBlock(BB), and the clone is wasted? I know there are many checks in MaybeCloneThreadableBasicBlock to ensure most cases it won't happen, but maybe it is helpful to add some assertion to catch unexpected case?
> 
> For example, because ProcessBlock(BB) is called in a loop, we can save the information about whether any clone has happend in the loop, and any threading has happened, and add an assertion after the loop to check if clone has happened, threading must happen as well.  
I understand your concern.

I am not sure if I like to save information for the next iteration.  That would get messy.  If you would like to see jump threading always happen after MaybeCloneThreadableBasicBlock, should I perform actual jump threading inside MaybeCloneThreadableBasicBlock? (Maybe we should change the function name at that point.)

I am wondering if it's an option to just "hope" that jump threading happens after MaybeCloneThreadableBasicBlock.  A couple of other helper transformations like TryToUnfoldSelectInCurrBB and ProcessBranchOnPHI also "hope" that they will increase jump threading opportunities.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:2122-2125
+  unsigned ZeroCount = 0;
+  unsigned OneCount = 0;
+  BasicBlock *ZeroPred = nullptr;
+  BasicBlock *OnePred = nullptr;
----------------
wmi wrote:
> Why we care Cst is one or zero? Can we use CstCount instead? 
The reason why I care about specific constants is that I would like to set up the CFG so that after calling MaybeCloneThreadableBasicBlock, taking the edge from NewBB to BB will necessarily imply which successor edge we take out of BB.

By CstCount, I assume you are proposing to count the number of incoming edges into PredBB with known successor edges out of BB.  If we do so, we could end up with CstCount == 2, where one incoming edge into PredBB takes the "then" edge out of BB, and the other takes the "else" edge.  That doesn't tell us which edge into PredBB we should redirect to NewBB.

Note that for simplicity, I would like to redirect exactly one incoming edge into PredBB to NewBB when no other incoming edge into PredBB is known to always take the same successor edge out of BB, which is why I care about ZeroCount == 1 or OneCount == 1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70247