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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked 3 inline comments as done.
fpetrogalli 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();
----------------
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`?


================
Comment at: llvm/lib/IR/Instructions.cpp:3242
+  TypeSize SrcBitSize = SrcTy->getScalarType()->getPrimitiveSizeInBits();
+  TypeSize DstBitSize = DstTy->getScalarType()->getPrimitiveSizeInBits();
 
----------------
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! :)



================
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;
----------------
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).`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76738