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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

It's a lot of code, and I am still reading it, but here's a bunch of mostly nits if you don't mind that... (while I am going to continue reading it).



================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:120
+    SmallVector<SelectInstToUnfold, 4> Stack;
+    for (SelectInstToUnfold SIToUnfold : SelectInsts) {
+      Stack.push_back(SIToUnfold);
----------------
Nit: don't need the brackets.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:133
+      // Put newly discovered select instructions into the work list.
+      for (const SelectInstToUnfold &NewSIToUnfold : NewSIsToUnfold) {
+        Stack.push_back(NewSIToUnfold);
----------------
Nit: don't need the brackets.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:215
+  if (SelectInst *SIOp = dyn_cast<SelectInst>(SI->getTrueValue())) {
+    assert(SIOp->hasOneUse());
+    TrueBlock = BasicBlock::Create(SI->getContext(), "si.unfold.true",
----------------
Create a helper function for this...


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:225
+  if (SelectInst *SIOp = dyn_cast<SelectInst>(SI->getFalseValue())) {
+    assert(SIOp->hasOneUse());
+    FalseBlock = BasicBlock::Create(SI->getContext(), "si.unfold.false",
----------------
... so that we don't need to duplicate this here.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:287
+    // Update the phi node of SI.
+    for (unsigned Idx = 0; Idx < SIUse->getNumIncomingValues(); ++Idx) {
+      if (SIUse->getIncomingBlock(Idx) == StartBlock) {
----------------
Nit: no need for all the curly brackets.

(sorry, I just find it a lot easier to read, and it's the coding style )


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:313
+  BasicBlock *BB;
+  uint64_t State;
+};
----------------
Perhaps a comment here that State just corresponds to the value of a case statement?


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:337
+    else
+      raw_string_ostream(BBName) << BB;
+    OS << BBName << " ";
----------------
Do we need BBName?
Can we not do something like:

  OS << BB->hasName() ? BB->getName() : "...";



================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:356
+
+  const BasicBlock *getDeterminatorBB() const { return DBB; }
+  void setDeterminator(const BasicBlock *BB) { DBB = BB; }
----------------
I was confused what exactly the determinator was, but I think it's the block that determines the next state. Think some comments about this would be beneficial too.



================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:472
+      return;
+    if (Instruction *I = dyn_cast<Instruction>(Val)) {
+      Q.push_back(I);
----------------
no curly brackets


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:479
+  bool isValidSelectInst(SelectInst *SI) {
+    if (!SI->hasOneUse()) {
+      return false;
----------------
Same


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:485
+    // The use of the select inst should be either a phi or another select.
+    if (!SIUse && !(isa<PHINode>(SIUse) || isa<SelectInst>(SIUse))) {
+      return false;
----------------
Same,

and actually in a lot more cases, so will stop mentioning it from now on, but there's opportunities to get rid of a lot of curly brackets. :-)


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:1146
+  for (BasicBlock &BB : F) {
+    if (auto *SI = dyn_cast<SwitchInst>(BB.getTerminator())) {
+
----------------
  auto *SI = dyn_cast<SwitchInst>(BB.getTerminator();
  if (!SI)
    continue;


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

https://reviews.llvm.org/D99205