[PATCH] D99205: Add jump-threading optimization for deterministic finite automata

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:78
+static cl::opt<unsigned> MaxPathDepth(
+    "dfa-max-path-length",
+    cl::desc("Max number of blocks searched to find threadable path"),
----------------
I think this option is not tested.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:83
+static cl::opt<unsigned>
+    CostThreshold("dfa-cost-threshold",
+                  cl::desc("Maximum cost accepted for the transformation"),
----------------
And this one too, so need some more tests for this.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:404
+  /// switch variable can be converted to unconditional jump.
+  bool isPredictable(const SwitchInst *SI) {
+    std::deque<Instruction *> Q;
----------------
I don't think we have tests for cases that are not predictable.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:458
+
+  bool isValidValue(Value *InpVal, SmallSet<Value *, 16> &SeenValues) {
+    if (SeenValues.find(InpVal) != SeenValues.end())
----------------
This could probably do with a more descriptive function name, to make more explicit what kind of Value we expect.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:495
+    BranchInst *SITerm = dyn_cast<BranchInst>(SIBB->getTerminator());
+    if (!SITerm || !SITerm->isUnconditional()) {
+      return false;
----------------
Don't need the brackets here


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:500
+    if (isa<PHINode>(SIUse) && SIBB->getSingleSuccessor() !=
+                                   dyn_cast<Instruction>(SIUse)->getParent()) {
+      return false;
----------------
and here.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:32
+//
+//===----------------------------------------------------------------------===//
+
----------------
alexey.zhikhar wrote:
> SjoerdMeijer wrote:
> > Just curious, is this algorithm described in literature? Can we include a reference?
> This is the algorithm that we came up with so we cannot provide a reference. "Codasip" has a [whitepaper](https://news.codasip.com/whitepaper-llvm-jump-threading) with a more general algorithm for jump threading that covers DFA as well. Justin @jkreiner [compared our approach with the one by Codasip](https://reviews.llvm.org/D88307), and found that we can cover all cases that Codasip can (for DFA) with one exception: currently our implementation is limited to cases in which the state variable of a switch is always predictable, but it can be extended to cover the case where the state variable is only sometimes predictable.
> 
> Thank you very much for your comments, I'm working on updating the diff, will submit it shortly.
Thanks for this, and for addressing my previous nitpicks about definitions and terminology.
I think it would be convenient to have most of them in one place, i.e. here, describing the high level ideas and terminology used.
Sketching a little bit, and copy-pasting from different places, I was thinking of something like this:

  // Transform each threading path to effectively jump thread the FSM. For
  // example the CFG below could be transformed as follows, where the cloned
  // blocks unconditionally branch to the next correct case based on what is
  // identified in the analysis.
  //          sw.bb                        sw.bb
  //        /   |   \                    /   |   \
  //   case1  case2  case3          case1  case2  case3
  //        \   |   /                 |      |      |
  //       determinator            det.2   det.3  det.1
  //        br sw.bb                /        |        \
  //                          sw.bb.2     sw.bb.3     sw.bb.1
  //                           br case2    br case3    br case1§
  //
  // Defintions and Terminology:
  //
  // * Threadable path:
  //   a list of basic blocks, the exit state, and the block that determines
  //   the next state, for which the following notation will be used:
  //   < path of BBs that form a cycle > [ state, determinator ]
  //
  // * Predictable switch:
  //   The switch variable is always a known constant so that all conditional
  //   jumps based on switch variable can be converted to unconditional jump.
  //
  // * Determinator:
  //   The basic block that determines the next state of the DFA. 
  //
  // ETC

And I think you can keep the comments at the different places inlined in the code, don't think that duplication will harm.


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

https://reviews.llvm.org/D99205