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

[Justin Kreiner via Phabricator via llvm-commits]

jkreiner marked 2 inline comments as done.
jkreiner added inline comments.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:109
+    DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Eager);
+    std::list<SelectInstToUnfold> Q;
+    for (SelectInstToUnfold SIToUnfold : SelectInsts) {
----------------
efriedma wrote:
> 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.
Okay, there's a few places std::list was used so I'll replace them all.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:182
+    NewSIsToUnfold->push_back(SelectInstToUnfold(SIOp, SIUse));
+    DTU->applyUpdates({{DominatorTree::Insert, TrueBlock, EndBlock}});
+  }
----------------
efriedma wrote:
> I don't think this applyUpdates() call does anything?  The DomTree doesn't care about unreachable edges.
Wouldn't it be needed since a new block is created? This edge is reached sometimes. I may be misunderstanding the usage of the DomTreeUpdater though.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:213
+  if (!TrueBlock) {
+    // A triangle pointing right.
+    TT = EndBlock;
----------------
efriedma wrote:
> Can you merge together the two triangle codepaths?  They appear nearly identical.  Probably just need a couple std::swap calls.
Sure I'll look into merging these two cases.


================
Comment at: llvm/lib/CodeGen/DFAJumpThreading.cpp:375
+      if (Case.getCaseSuccessor() == BB) {
+        return Case.getCaseValue();
+      }
----------------
efriedma wrote:
> Do you check somewhere that there's exactly one ConstantInt associated with a given successor?  In general, there could be any number.
Good point, this case isn't checked for so it would currently cause buggy behavior in this function.

Now that I think about it, this function can probably be removed since the return value stored as the EntryValue isn't used in the transformation.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetMachine.cpp:461
+  if (TM->getOptLevel() >= CodeGenOpt::Default)
+    addPass(createDFAJumpThreadingPass(TM));
+
----------------
efriedma wrote:
> 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.
I agree that it may be beneficial to run the pass earlier. I'm not aware yet of which optimizations it could interfere with. Do you have any suggestions of where to run it instead?


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

https://reviews.llvm.org/D99205