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

[Alexey Zhikhartsev via Phabricator via llvm-commits]

alexey.zhikhar marked 14 inline comments as done.
alexey.zhikhar added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:102
+                                     cl::desc("Enable DFA jump threading."),
+                                     cl::init(true), cl::Hidden);
+
----------------
SjoerdMeijer wrote:
> Just a thought on "deploying" this. Perhaps easier to start having this off by default? 
> In case  problems are found after committing (correctness, or perhaps compile times), you don't need to revert the whole pass, but instead just toggle this and can keep the pass in tree.
Good idea, done.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:72
+static cl::opt<bool>
+    IsViewCFGBefore("dfa-jump-view-cfg-before",
+                    cl::desc("View the CFG before DFA Jump Threading"),
----------------
ChuanqiXu wrote:
> IsViewCFGBefore => WouldViewCFGBeforeTransform or ViewCFGBeforeTransform.
> 
> Now the name looks a little bit confusing.
I usually name all my boolean variables `IsSomething` but I agree that `IsView...` sounds awkward. Also having a verb in the beginning make it sound like a routine rather than a variable. I updated it to `ClViewCFGBefore`, `Cl` for command line.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:381
+
+  virtual ~MainSwitch() = default;
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:722
+    unsigned CondBranches =
+        APInt(32, Switch->getNumSuccessors()).ceilLogBase2();
+    unsigned NumInsts = Metrics.NumInsts;
----------------
ChuanqiXu wrote:
> What's the rationale to use ceil log base 2? Maybe  there is a conclusion in math. But I guess it may be better to tell it.
Added a comment.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:739
+
+  /// Transform each threading path to effectively jump thread the FSM. For
+  /// example the CFG below could be transformed as follows, where the cloned
----------------
SjoerdMeijer wrote:
> Could you include here, or where ever most appropiate, the definition of threading path. Copied from a test case:
> 
>   A threadable path includes a list of basic blocks, the exit
>   state, and the block that determines the next state.
>   < path of BBs that form a cycle > [ state, determinator ]
Added a description to the definition of the `ThreadingPath` class.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:1138
+
+  for (Function::iterator B = F.begin(), BE = F.end(); B != BE; B++) {
+    if (auto *SI = dyn_cast<SwitchInst>(B->getTerminator())) {
----------------
ChuanqiXu wrote:
> It may be better to comment why we don't use range-based for here.
No good reason, fixed.


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

https://reviews.llvm.org/D99205