[PATCH] D38313: [InstCombine] Introducing Pattern Instruction Combine plug-in into InstCombine pass

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

Do you have tests for "Truncating to different width than the original trunc instruction requires. (this is useful if we reduce the expression width, even if we are not eliminating any instruction)."



================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:215
+  // If we already calculated the minimum bit-width for this valid bit-width, or
+  // for a smaler valid bit-width, then just return the answer we already have.
+  if (Info.ValidBitWidth >= ValidBitWidth)
----------------
smaller*


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:223
+  // Must update both valid bit-width and minimum bit-width at this point, to
+  // prevent infinit loop, when this instruction is part of a loop in the dag.
+  Info.ValidBitWidth = ValidBitWidth;
----------------
infinit*


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:252
+  // If minimum bit-width is equal to valid bit-width, and original bit-width is
+  // not legal integer, then use the target destenation type. Otherwise, use
+  // the smallest integer type in the range [MinBitWidth, OrigBitWidth).
----------------
destination*


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:255
+  Type *NewDstSclTy = DstTy->getScalarType();
+  if (DL.isLegalInteger(OrigBitWidth) || MinBitWidth > ValidBitWidth) {
+    NewDstSclTy = DL.getSmallestLegalIntType(DstTy->getContext(), MinBitWidth);
----------------
Does this call isLegalInteger using the scalar bit width for vectors? Not sure that's valid.


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:259
+      return nullptr;
+    // update minimum bit-width with the new destenation type bit-width.
+    MinBitWidth = NewDstSclTy->getScalarSizeInBits();
----------------
destination*


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:397
+  // Process all TruncInst in the Worklist, for each instruction:
+  //   1. Check if we it domenates an eligible expression dag to be reduced.
+  //   2. Create a reduced expression dag and replace the old one with it.
----------------
dominates*

"we" looks unnecessarry


https://reviews.llvm.org/D38313