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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:62
+
+bool AggressiveInstCombiner::runOnFunction(Function &F) {
+  auto &TLI = getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
----------------
What about the new pass manager?


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombineInternal.h:89
+
+  /// Calculate the minimum number of LSBs needed to generate the number of
+  /// BitWidth in the /p CurrentTruncInst destination.
----------------
This sentence reads funny


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:80
+
+    Instruction *I = dyn_cast<Instruction>(Curr);
+    if (!I)
----------------
Can you consistently use auto with dyn_cast throughout this patch.


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:175
+      for (auto *Operand : Operands)
+        if (auto IOp = dyn_cast<Instruction>(Operand))
+          Info.MinBitWidth =
----------------
LLVM style preferes "auto *IOp". We don't want auto to hide the fact that its a pointer. Please scrub the whole patch for this.


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:206
+    // Use the smallest integer type in the range [MinBitWidth, OrigBitWidth).
+    Type *Ty = DL.getSmallestLegalIntType(DstTy->getContext(), MinBitWidth);
+    // Update minimum bit-width with the new destination type bit-width if
----------------
In the case of vectors, is this using legal scalar integer types to constrain the vector element type? I'm not sure if that's the right behavior. The legal scalar types don't necessarily imply anything about vector types.

I think we generally try to avoid creating vector types that didn't appear in the IR.


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:210
+    MinBitWidth = Ty ? Ty->getScalarSizeInBits() : OrigBitWidth;
+  } else { // MinBitWidth == ValidBitWidth
+    // In this case the expression can be evaluated with the trunc instruction
----------------
Isn't this MinBitWidth == TruncBitWidth?


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:750
   // calls, etc, so let instcombine do this.
+  PM.add(createAggressiveInstCombinerPass());
   addInstructionCombiningPass(PM);
----------------
What about the new pass builder for the new pass manager?


https://reviews.llvm.org/D38313