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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

I am making an AggressiveJumpThreading pass in  downstream to solve the State Machine in Coremark.
And I omit the problem that more aggressive jump threading would cause irreducible control flow. I heard about that gcc has implemented a version which overcomes the problem. It may be beneficial to look into the details of the gcc implementation for all of us.
This patch looks like a big pattern match to me. It requires a switch statement whose condition are predictable, in other words, DFA.
First question is that is it profitable to make a pass for specific pattern? Since I feel that the DFA pattern wouldn't be normal in codes. Then if the pass turned off by default, I have no problems.

The second one is that this pass would collect all paths available in AllSwitchPaths and considering cost model when performing the transformation. 
One concern is that if it may be possible wasting too many times in collecting useless paths who is cost is more than benefit.



================
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"),
----------------
IsViewCFGBefore => WouldViewCFGBeforeTransform or ViewCFGBeforeTransform.

Now the name looks a little bit confusing.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:261-262
+    // Update any other PHI nodes in EndBlock.
+    for (auto II = EndBlock->begin(); PHINode *Phi = dyn_cast<PHINode>(II);
+         ++II) {
+      if (Phi != SIUse) {
----------------
It may be suitable to use range based for.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:381
+
+  virtual ~MainSwitch() = default;
+
----------------
Does it necessary to mark `~MainSwitch` as virtual? Since there is no derived class from MainSwitch.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:722
+    unsigned CondBranches =
+        APInt(32, Switch->getNumSuccessors()).ceilLogBase2();
+    unsigned NumInsts = Metrics.NumInsts;
----------------
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.


================
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())) {
----------------
It may be better to comment why we don't use range-based for here.


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

https://reviews.llvm.org/D99205