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

[Zvi Rackover via Phabricator via llvm-commits]

zvi added inline comments.


================
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
----------------
craig.topper wrote:
> 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.
That' s a good point. Will look into this.


================
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
----------------
craig.topper wrote:
> Isn't this MinBitWidth == TruncBitWidth?
Your observation is correct but the comment is also correct and it explains something that may not be obvious.
At this point, we completed visiting the expression tree starting from the truncate's operand and found 'MinBitWidth' by taking the max of all min-bit-width requirements of the predecessors. Now, since this is a truncate instruction, and by construction of the expression tree it follows that the computed MinBitWidth can never be less than the truncate's return types's size in bits or in other words MinBitWidth >= TruncBitWidth.
The case of MinBitWidth > TruncBitWidth is hand;ed in the then block just above, and what remains in to handle the MinBitWidth == TruncBitWidth in the else block here.
I can put this in a comment if you think it would be helpful.


https://reviews.llvm.org/D38313