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

[Alexey Zhikhartsev via Phabricator via llvm-commits]

alexey.zhikhar marked 7 inline comments as done.
alexey.zhikhar 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"),
----------------
SjoerdMeijer wrote:
> I think this option is not tested.
A test case for `MaxPathDepth` is not ready yet, but duly noted. Thanks


================
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;
----------------
SjoerdMeijer wrote:
> I don't think we have tests for cases that are not predictable.
Please see `negative4`


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:32
+//
+//===----------------------------------------------------------------------===//
+
----------------
SjoerdMeijer wrote:
> 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.
Thanks, Sjoerd, I added this to the file header. Generally I prefer to not duplicate documentation for the same reasons as duplicating code, so I removed the long comment from `TransformDFA::createAllExitPaths()` but I can put it back if you insist.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:381
+
+  virtual ~MainSwitch() = default;
+
----------------
ChuanqiXu wrote:
> alexey.zhikhar wrote:
> > ChuanqiXu wrote:
> > > Does it necessary to mark `~MainSwitch` as virtual? Since there is no derived class from MainSwitch.
> > Classes might derive from `MainSwitch` in the future, it's a good practice to declare destructors virtual.
> So it may be better to add `virtual ~MainSwitch` in the future.
Let's see if Sjoerd @SjoerdMeijer has an opinion here.


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

https://reviews.llvm.org/D99205