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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Did a first pass, see comments inline. Plan to look at this closer soon.

I think some tests are missing, mostly negative tests:

- test for min code size,
- test with blocks that cannot be cloned,
- test with some unsupported instructions.



================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:102
+                                     cl::desc("Enable DFA jump threading."),
+                                     cl::init(true), cl::Hidden);
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:12
+// finite automaton, we can jump thread the switch statement reducing number of
+// conditional jumps. Currently this does not happen in LLVM jump threading.
+// For example the code pattern on the left could functionally be transformed
----------------
Nit: perhaps you can be more specific here:

  Currently this does not happen in LLVM jump threading

and say this is JumpThreading.cpp?


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:24
+//
+// The pass first checks that a switch variable is decided by the control flow
+// path taken in the loop. It then enumerates through all paths in the loop and
----------------
Nit picking some words here. What do you mean by "switch variable" and what do you mean by "decided"?


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:29
+// Using this information it creates new paths that unconditionally branch to
+// the next case. This involves cloning code, so it only runs if the amount of
+// code duplicated is below a threshold.
----------------
Nit: runs -> triggers?


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:32
+//
+//===----------------------------------------------------------------------===//
+
----------------
Just curious, is this algorithm described in literature? Can we include a reference?


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:185
+
+/// Unfold the select instruction held in \p SIToUnfold.
+///
----------------
Can you define what unfolding means in this context?


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:190
+///
+/// This routine is inspired by CodeGenPrepare::optimizeSelectInst().
+void unfold(DomTreeUpdater *DTU, SelectInstToUnfold SIToUnfold,
----------------
Do you mean that this is code that could be shared? Should this be a TODO?


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:321
+// This data structure keeps track of all blocks that have been cloned in the
+// optimization. If two different ThreadingPaths clone the same block for a
+// certain state it should be reused, and it can be looked up in this map.
----------------
  // This data structure keeps track of all blocks that have been cloned in the
  // optimization.

-> 

  // This data structure keeps track of all blocks that have been cloned.



================
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
----------------
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 ]


================
Comment at: llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-transform.ll:19
+; CHECK: for.body.jt2:
+; CHECK-NEXT: %count.jt2
+; CHECK-NEXT: %state.jt2
----------------
Perhaps check the full IR for clarity.


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

https://reviews.llvm.org/D99205