[PATCH] D76738: [llvm][IR][CastInst] Update `castIsValid` for scalable vectors.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/IR/Instructions.cpp:3241
   // Get the size of the types in bits, we'll need this later
-  unsigned SrcBitSize = SrcTy->getScalarSizeInBits();
-  unsigned DstBitSize = DstTy->getScalarSizeInBits();
+  TypeSize SrcBitSize = SrcTy->getScalarType()->getPrimitiveSizeInBits();
+  TypeSize DstBitSize = DstTy->getScalarType()->getPrimitiveSizeInBits();
----------------
fpetrogalli wrote:
> Nit: While doing some of the modification you have asked me, I got confused by the fact that the variables `[Src|Dst]BitSize` are referring to the size of the scalars, not of the whole `[Src|Dst]Ty`. Is it OK if I rename them to  `[Src|Dst]ScalarBitSize`?
Sure.


================
Comment at: llvm/lib/IR/Instructions.cpp:3242
+  TypeSize SrcBitSize = SrcTy->getScalarType()->getPrimitiveSizeInBits();
+  TypeSize DstBitSize = DstTy->getScalarType()->getPrimitiveSizeInBits();
 
----------------
fpetrogalli wrote:
> efriedma wrote:
> > What's wrong with getScalarSizeInBits()?
> `getScalarSizeInBits` implicitly casts the `TypeSize` of `getScalarType()->getPrimitiveSizeInBits()` into an integer, an operation that we have agreed to mark as deprecated. I probably should have mentioned this in the commit message.
> 
> `getScalarSizeInBits` is implemented as follows:
> 
> ```
>  unsigned Type::getScalarSizeInBits() const {
>    return getScalarType()->getPrimitiveSizeInBits();
>  }
>  ```
> 
> Given that we don't expect "scalar" values to be scalable, I can revert this change to use `unsigned` values, and maybe submit a separate patch that adds an assertion inside  `getScalarSizeInBits` that makes sure to check that the TypeSize retrieved by `getPrimitiveSizeInBits` is not a scalable `TypeSize`.
> 
> The method will look like this after the change (disclaimer, I didn't compile it!):
> 
> ```
>  unsigned Type::getScalarSizeInBits() const {
>    TypeSize TS = getScalarType()->getPrimitiveSizeInBits();
>    assert(!TS.isScalable() && "Invalid type.")
>    return TS.getKnownMinSize();
>  }
> ```
> 
> 
> Please let me know which of the following you want me to do:
> 
> 1. mention and justify the change in the commit message
> 2. revert the change to use unsigned, and create a separate patch that add the assertion "is not scalable" to `getScalarSizeInBits`.
> 3. third option you might have that I have missed! :)
> 
I think it makes sense to keep using getScalarSizeInBits(), and fix the implementation like you suggest.

You can use getFixedSize() instead of separately asserting !isScalable().


================
Comment at: llvm/lib/IR/Instructions.cpp:3284
     if (VectorType *VT = dyn_cast<VectorType>(SrcTy))
-      if (VT->getNumElements() != cast<VectorType>(DstTy)->getNumElements())
+      if (VT->getElementCount() != cast<VectorType>(DstTy)->getElementCount())
         return false;
----------------
fpetrogalli wrote:
> efriedma wrote:
> > Can we simplify some of the getElementCount() calls by using SrcLength/DstLength instead?
> > Can we simplify some of the getElementCount() calls by using SrcLength/DstLength instead?
> 
> Yes, on it. 
> 
> How about also creating two boolean vars that hold the `isa<VectorType>([Src|Dst]Ty)` and use them instead of the `isa<VectorType>([Src|Dst]Ty)` or `dyn_cast<VectorType>([Src|Dst]Ty).`?
Whatever you think is more readable is fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76738/new/

https://reviews.llvm.org/D76738