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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:13
+// conditional jumps. Currently this does not happen in LLVM jump threading.
+//
+//===----------------------------------------------------------------------===//
----------------
Could probably use a brief outline of the overall algorithm here.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:109
+    DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Eager);
+    std::list<SelectInstToUnfold> Q;
+    for (SelectInstToUnfold SIToUnfold : SelectInsts) {
----------------
Don't use std::list.

I think you can use SmallVector and pop_back(), assuming the iteration order doesn't matter.  If you need LIFO order, use std::deque.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:182
+    NewSIsToUnfold->push_back(SelectInstToUnfold(SIOp, SIUse));
+    DTU->applyUpdates({{DominatorTree::Insert, TrueBlock, EndBlock}});
+  }
----------------
I don't think this applyUpdates() call does anything?  The DomTree doesn't care about unreachable edges.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:213
+  if (!TrueBlock) {
+    // A triangle pointing right.
+    TT = EndBlock;
----------------
Can you merge together the two triangle codepaths?  They appear nearly identical.  Probably just need a couple std::swap calls.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:375
+      if (Case.getCaseSuccessor() == BB) {
+        return Case.getCaseValue();
+      }
----------------
Do you check somewhere that there's exactly one ConstantInt associated with a given successor?  In general, there could be any number.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:450
+    // If this is a function argument or another non-instruction, then give up.
+    Instruction *I = dyn_cast<Instruction>(InpVal);
+    if (I == nullptr)
----------------
isa<Instruction>() if you're not going to use the pointer returned by dyn_cast<>.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:690
+    CodeMetrics::collectEphemeralValues(
+        SwitchPaths->getSwitchBlock()->getParent(), AC, EphValues);
+
----------------
Probably want to ensure we're only calling collectEphemeralValues once per function.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:740
+    unsigned NumInsts = Metrics.NumInsts;
+    unsigned DuplicationCost = NumInsts / CondBranches;
+
----------------
jkreiner wrote:
> Here is the cost calculation we implemented. It could be more accurate by accounting for instructions and blocks that get simplified, but for now it at least prevents code explosion. 
Okay, makes sense.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetMachine.cpp:461
+  if (TM->getOptLevel() >= CodeGenOpt::Default)
+    addPass(createDFAJumpThreadingPass(TM));
+
----------------
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.


================
Comment at: llvm/test/CodeGen/AArch64/dfa-constant-propagation.ll:1
+; RUN: opt -S -dfa-jump-threading -sccp -simplifycfg %s | FileCheck %s
+
----------------
Please put the tests in llvm/test/Transforms/DFAJumpThreading.


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

https://reviews.llvm.org/D99205