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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombinePatterns.cpp:255
+  Type *NewDstSclTy = DstTy->getScalarType();
+  if (DL.isLegalInteger(OrigBitWidth) || MinBitWidth > ValidBitWidth) {
+    NewDstSclTy = DL.getSmallestLegalIntType(DstTy->getContext(), MinBitWidth);
----------------
aaboud wrote:
> craig.topper wrote:
> > Does this call isLegalInteger using the scalar bit width for vectors? Not sure that's valid.
> Can you explain, what you mean by "that's not valid"?
> Are you referring to the algorithm?
> 
> Are you concerned of cases where a scalar type is legal (e.g. i32), while a vector type is not (e.g. <8xi32>), or the opposite direction?
> 
> I believe that my algorithm should not mind about the number of elements in the type, only about the width.
> The reason for this check, is to keep the old case, where we will not reduce a legal to non-legal type.
> 
> Do you think that we should do a more accurate check?
I guess my point was just that the legality of the scalar type is unrelated to the legality of the vector type. In x86 for example. i64 isn't legal in 32-bit mode, but v2i64 is. If I remember right the existing code doesn't even call isLegalInteger for vector types? I'd check but someone is hammering the server I normally use.


https://reviews.llvm.org/D38313