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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

In D99205#2866668 <https://reviews.llvm.org/D99205#2866668>, @SjoerdMeijer wrote:

> In D99205#2866301 <https://reviews.llvm.org/D99205#2866301>, @ChuanqiXu wrote:
>
>> I am making an AggressiveJumpThreading pass in  downstream to solve the State Machine in Coremark.
>
> We have 2 aggressive jump threading passes in upstream review: this work, and also D88307 <https://reviews.llvm.org/D88307>. If I understand this correctly, you're working on another downstream? If so, would it not be more efficient if we support this work? For me, this work has the most potential as it is under active development as opposed to D88307 <https://reviews.llvm.org/D88307>. Your remark about irreducible control flow is a valid one, which was made earlier too. Eli said this about that:
>
>> This is kind of late to be running this pass. That might be useful in some cases: we might be making the code harder to analyze if there's irreducible control flow. On the other hand, we're skipping interesting optimizations if the CFG becomes significantly simpler. My instinct is that the advantages of running earlier outweigh the disadvantages.
>
> which is something that still needs to be figured out I think. I.e., it's run earlier now, but do we need more performance numbers to see if this doesn't regress other stuff? But @ChuanqiXu, if you're working on something similar and have ideas, please consider sharing them here.

Since I only started to see Coremark recently and you have much more experience. I should be fine if there isn't any regression found.

>> 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.
>
> My colleague @jaykang10 found that this also triggers on Perlbench in SPEC.

Cool, I would try to measure it.



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


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:722
+    unsigned CondBranches =
+        APInt(32, Switch->getNumSuccessors()).ceilLogBase2();
+    unsigned NumInsts = Metrics.NumInsts;
----------------
alexey.zhikhar wrote:
> 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.
Is this assumption stable? For the one hand, the motivation example in Coremark wouldn't be lowered Into a binary tree. On the other hand, it may be possible that the switch statement may be lowered to a jump table.


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

https://reviews.llvm.org/D99205