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

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added a comment.

Thanks Zvi for addressing all comments and questions while I am away.
Craig, please, see answers for your questions inlined below.

Thanks,
Amjad



================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:205
+  if (MinBitWidth > TruncBitWidth) {
+    // Use the smallest integer type in the range [MinBitWidth, OrigBitWidth).
+    Type *Ty = DL.getSmallestLegalIntType(DstTy->getContext(), MinBitWidth);
----------------
craig.topper wrote:
> I'm not sure you've addressed my vector concerns. The first part of this 'if' would create a new type for vectors by using getSmallestLegalIntType.
I think that in the "else" part, the one that I kept the same behavior as the original instcombine code, we might end up creating a vector type that was not in the IR, as for vector types we do not check for type legality.
So, why in this case we should behave differently?

Regarding the scalar check, it might be redundant, but not always, because even if the "trunc" instruction is performed on vector type, the evaluated expression might contain scalar operations (due to the "insertelement" instruction, which will be supported in next few patches).

Furthermore, my assumption is that codegen legalizer will promote the illegal vector type back to the original type (or to a smaller one), in both cases we will not get worse code than the one we started with!
Is that assumption too optimistic?


================
Comment at: lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:217
+    bool ToLegal = MinBitWidth == 1 || DL.isLegalInteger(MinBitWidth);
+    if (FromLegal && !ToLegal)
+      return OrigBitWidth;
----------------
craig.topper wrote:
> I think the !Vector check that was here was correct previously. We don't do isLegalnteger checks on the scalar types of vectors. For vectors we assume that if the type was present in the IR, the transform is fine. In this block, TruncWidth == MinBitWidth so the type existed in the original IR. My vector concerns were about the block above where we create new a new type.
I agree with Craig, need to change back to:

```
if (!DstTy->isVectorTy() && FromLegal && !ToLegal)
``` 


https://reviews.llvm.org/D38313