[PATCH] D38313: [InstCombine] Introducing Aggressive Instruction Combine pass

[Zvi Rackover via Phabricator via llvm-commits]

zvi added a comment.

Some more minor comments



================
Comment at: docs/Passes.rst:647
+``-aggressive-instcombine``: Combine expression patterns
+------------------------------------------------
+
----------------
For the sake of consistency with other title, please add more dashes for alignment with title.


================
Comment at: docs/Passes.rst:649
+
+Combine expression pattern to form expression with fewer, simple instructions.
+This pass does not modify the CFG.
----------------
*patterns *expressions


================
Comment at: docs/Passes.rst:652
+
+For example, this pass reduce width of expression post-dominated by TruncInst
+into smaller width when applicable.
----------------
*reduces the width of expressions


================
Comment at: docs/Passes.rst:655
+
+It differs from instcombine pass in that it contains pattern optimization that requires
+higher complexity than the O(1), thus, it should run fewer times than instcombine pass.
----------------
Maybe also say that the pattern-scan may cover the entire functions as opposed to the locality-limited instcombine patterns?


================
Comment at: include/llvm/InitializePasses.h:284
 void initializePatchableFunctionPass(PassRegistry&);
+void initializeAggressiveInstCombinerPass(PassRegistry&);
 void initializePeepholeOptimizerPass(PassRegistry&);
----------------
These declarations are not perfectly sorted, but still maybe try to move this up.


================
Comment at: include/llvm/Transforms/Scalar.h:137
 //
+// AggressiveInstCombiner - Combine expression pattern to form expression with
+// fewer, simple instructions. This pass does not modify the CFG.
----------------
*expressions *patterns


================
Comment at: lib/LTO/LLVMBuild.txt:30
  InstCombine
+ AggressiveInstCombine
  Linker
----------------
Please preserve sorted ordering


================
Comment at: lib/Passes/LLVMBuild.txt:22
 parent = Libraries
-required_libraries = Analysis CodeGen Core IPO InstCombine Scalar Support TransformUtils Vectorize Instrumentation
+required_libraries = Analysis CodeGen Core IPO InstCombine AggressiveInstCombine Scalar Support TransformUtils Vectorize Instrumentation
----------------
Please preserve sorted ordering


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:31
+/// This class provides both the logic to combine expression patterns and
+/// combine them. It differs from InstCombiner class in that it runs only once,
+/// which allows pattern optimization to have higher complexity than the O(1)
----------------
Can you please rephrase this so that it is obvious that the pass can be run more than once and that the internal mechanism limits pattern matchers to run once? Maybe something like this

```
It differs than  the Instcombine in that each pattern combiner is run only once as opposed to instcombine's multi-iteration ...
```


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:54
+void AggressiveInstCombiner::getAnalysisUsage(AnalysisUsage &AU) const {
+  AU.setPreservesCFG();
+  AU.addRequired<TargetLibraryInfoWrapperPass>();
----------------
Is there an importance of order to the calls? If not, maybe group the set/addPreserve* calls and addRequired call to two group separated by a newline. IMHO it's more readable.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombineInternal.h:21
+#include "llvm/Analysis/GlobalsModRef.h"
+#include "llvm/Analysis/ConstantFolding.h"
+#include "llvm/Analysis/TargetLibraryInfo.h"
----------------
Move two lines up


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombineInternal.h:69
+  };
+  /// An ordered map representing expression dag dominated by current processed
+  /// TruncInst. It maps each instruction in the dag to its Info structure.
----------------
post-dominated


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:40
+/// \p New instruction DebugLoc and Name with those of \p Old instruction.
+static void InsertAndUpdateNewInstWith(Instruction &New, Instruction &Old) {
+  assert(!New.getParent() &&
----------------
+1 for preserving DebugInfo. Is there other metadata that may need to be copied? I can't think of anything in particular, just wanted to raise the possibility.


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:44
+  BasicBlock *BB = Old.getParent();
+  BB->getInstList().insert(Old.getIterator(), &New); // Insert inst
+  New.setDebugLoc(Old.getDebugLoc());
----------------
Is it possible to reuse llvm::ReplaceInstWithInst instead of doing the low-level work explicitly?


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:54
+  switch (Opc) {
+  case Instruction::Trunc:
+  case Instruction::ZExt:
----------------
IIRC, in one of the previous revisions of this patch there was a comment explaining why these cast instructions are skipped? Can you please revive it or add a new one?


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:64
+  case Instruction::Xor:
+    Ops.push_back(I->getOperand(0));
+    Ops.push_back(I->getOperand(1));
----------------
Could we avoid pushing constants and maybe even generalize to avoid values that are not instructions? At least for these cases it may be ok, not for others, such as divide where we need to be more cautious


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:202
+        // answer we already calculated.
+        if (InstInfoMap[IOp].ValidBitWidth >= ValidBitWidth)
+          continue;
----------------
At this point a mappting for IOp must exist in InstInfoMap, right? Then please use lookup() or find(). Also better avoid searching for same key more than once.


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:209
+  }
+  unsigned MinBitWidth = InstInfoMap[cast<Instruction>(Src)].MinBitWidth;
+  assert(MinBitWidth >= TruncBitWidth);
----------------
lookup()


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:227
+    if (!DstTy->isVectorTy() && FromLegal && !ToLegal)
+      return OrigBitWidth;
+  }
----------------
IMO this would be more readable and guarantee that code changes won't lead to uses of stale values of MinBitWidth:

```
MinBitWidth = OrigBitwidth;
```
and drop the return


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:352
+    InstInfoMap[I].NewValue = Res;
+    InsertAndUpdateNewInstWith(*Res, *I);
+  }
----------------
I may have missed this, but why defer insertion of instruction to the BB to this point? And consider using the IRBuilder instead of calling ::Create* above and below.


https://reviews.llvm.org/D38313