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

[Amjad Aboud via Phabricator via llvm-commits]

aaboud 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);
----------------
craig.topper wrote:
> 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.
I agree that I have a logical bug here.
I was trying to keep the same semantic of original code:

```
if ((DestTy->isVectorTy() || shouldChangeType(SrcTy, DestTy)) &&
      canEvaluateTruncated(Src, DestTy, *this, &CI)) {
```

I guess, I need to ignore vector types. (as in above code) and fix the logic to this:

```
    if (MinBitWidth > ValidBitWidth) {
      Type *Ty = DL.getSmallestLegalIntType(DstTy->getContext(), MinBitWidth);
      if (!Ty)
        return nullptr;
      // update minimum bit-width with the new destination type bit-width.
      MinBitWidth = Ty->getScalarSizeInBits();
    } else { // MinBitWidth == ValidBitWidth
      if (!DestTy->isVectorTy() && !shouldChangeType(SrcTy, DestTy))
        return nullptr;
    }
    Type *NewDstSclTy = IntegerType::get(DstTy->getContext(), MinBitWidth);
```




https://reviews.llvm.org/D38313