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

[fpetrogalli]

I have updated the whole method CastInst::castISValid to correctly
support scalable vectors.

[fpetrogalli]

This comment has been deleted.

[efriedma]

llvm/lib/IR/Instructions.cpp

3245

What's wrong with getScalarSizeInBits()?

3287–3288

Can we simplify some of the getElementCount() calls by using SrcLength/DstLength instead?

[fpetrogalli]

llvm/lib/IR/Instructions.cpp

3244–3249

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?

3245

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! :)

3287–3288

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).?

[efriedma]

llvm/lib/IR/Instructions.cpp

3244–3249

Sure.

3245

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().

3287–3288

Whatever you think is more readable is fine.

[fpetrogalli]

Hi @efriedma,

thank you for your review!

I have addressed all your comments, and applied the following changes:

1. restored the use of unsigned for the size in bits. I have created a

separate patch that updated the unsigned Type::getScalarSizeInBits()
to use TypeSize::getFixedSize(): https://reviews.llvm.org/D76892

2. I have fixed the tests that were refusing to bitcast fixed size

vectors to scalable vectors (and viceversa). In the previous patch I
erroneusly used scalable vectors everywhere, just with different
lanes.

3. I have replaced the use of getVectorElementCount with variables

that are set at the beginning of the method.

4. I have renamed [Src|Dst]BitSize to [Src|Dst]ScalarBitSize, to

avoid confusion when dealing with vector types instead of scalars.

Kind regards,

Francesco

[fpetrogalli]

Last but not least, I have renamed [Src|Dst]Length into [Src|Dst]EC as the variables now hold ElementCount and not unsigned.

[efriedma]

llvm/lib/IR/Instructions.cpp

3286

"if (SrcIsVec != DstIsVec)" is redundant, here and for IntToPtr.

3332–3333

Remove "if (SrcIsVec)", and just do "return SrcEC == DstEC;"?

[fpetrogalli]

Hi @efriedma,

I have further simplified the code according to your comments.

Thank you!

Francesco

[efriedma]

LGTM

[fpetrogalli]

Rebasing on top of master. [NFC]

[fpetrogalli]

This comment was removed by fpetrogalli.

[fpetrogalli]

Rebase on top of master.

llvm/lib/IR/Instructions.cpp

 CastInst::castIsValid(Instruction::CastOps op, Value *S, Type *DstTy) {
   // Check for type sanity on the arguments
   Type *SrcTy = S->getType();
 
   if (!SrcTy->isFirstClassType() || !DstTy->isFirstClassType() ||
       SrcTy->isAggregateType() || DstTy->isAggregateType())
     return false;
 
-  // Get the size of the types in bits, we'll need this later
-  unsigned SrcBitSize = SrcTy->getScalarSizeInBits();
-  unsigned DstBitSize = DstTy->getScalarSizeInBits();
+  // Get the size of the types in bits, and whether we are dealing
+  // with vector types, we'll need this later.
+  bool SrcIsVec = isa<VectorType>(SrcTy);
+  bool DstIsVec = isa<VectorType>(DstTy);
+  unsigned SrcScalarBitSize = SrcTy->getScalarSizeInBits();
+  unsigned DstScalarBitSize = DstTy->getScalarSizeInBits();
 
   // If these are vector types, get the lengths of the vectors (using zero for
   // scalar types means that checking that vector lengths match also checks that
   // scalars are not being converted to vectors or vectors to scalars).
-  unsigned SrcLength = SrcTy->isVectorTy() ?
-    cast<VectorType>(SrcTy)->getNumElements() : 0;
-  unsigned DstLength = DstTy->isVectorTy() ?
-    cast<VectorType>(DstTy)->getNumElements() : 0;
+  ElementCount SrcEC = SrcIsVec ? cast<VectorType>(SrcTy)->getElementCount()
+                                : ElementCount(0, false);
+  ElementCount DstEC = DstIsVec ? cast<VectorType>(DstTy)->getElementCount()
+                                : ElementCount(0, false);
 
   // Switch on the opcode provided
   switch (op) {
   default: return false; // This is an input error
   case Instruction::Trunc:
     return SrcTy->isIntOrIntVectorTy() && DstTy->isIntOrIntVectorTy() &&
-      SrcLength == DstLength && SrcBitSize > DstBitSize;
+           SrcEC == DstEC && SrcScalarBitSize > DstScalarBitSize;
   case Instruction::ZExt:
     return SrcTy->isIntOrIntVectorTy() && DstTy->isIntOrIntVectorTy() &&
-      SrcLength == DstLength && SrcBitSize < DstBitSize;
+           SrcEC == DstEC && SrcScalarBitSize < DstScalarBitSize;
   case Instruction::SExt:
     return SrcTy->isIntOrIntVectorTy() && DstTy->isIntOrIntVectorTy() &&
-      SrcLength == DstLength && SrcBitSize < DstBitSize;
+           SrcEC == DstEC && SrcScalarBitSize < DstScalarBitSize;
   case Instruction::FPTrunc:
     return SrcTy->isFPOrFPVectorTy() && DstTy->isFPOrFPVectorTy() &&
-      SrcLength == DstLength && SrcBitSize > DstBitSize;
+           SrcEC == DstEC && SrcScalarBitSize > DstScalarBitSize;
   case Instruction::FPExt:
     return SrcTy->isFPOrFPVectorTy() && DstTy->isFPOrFPVectorTy() &&
-      SrcLength == DstLength && SrcBitSize < DstBitSize;
+           SrcEC == DstEC && SrcScalarBitSize < DstScalarBitSize;
   case Instruction::UIToFP:
   case Instruction::SIToFP:
     return SrcTy->isIntOrIntVectorTy() && DstTy->isFPOrFPVectorTy() &&
-      SrcLength == DstLength;
+           SrcEC == DstEC;
   case Instruction::FPToUI:
   case Instruction::FPToSI:
     return SrcTy->isFPOrFPVectorTy() && DstTy->isIntOrIntVectorTy() &&
-      SrcLength == DstLength;
+           SrcEC == DstEC;
   case Instruction::PtrToInt:
-    if (isa<VectorType>(SrcTy) != isa<VectorType>(DstTy))
-      return false;
-    if (VectorType *VT = dyn_cast<VectorType>(SrcTy))
-      if (VT->getNumElements() != cast<VectorType>(DstTy)->getNumElements())
-        return false;
+    if (SrcEC != DstEC)
     return SrcTy->isPtrOrPtrVectorTy() && DstTy->isIntOrIntVectorTy();
   case Instruction::IntToPtr:
-    if (isa<VectorType>(SrcTy) != isa<VectorType>(DstTy))
-      return false;
-    if (VectorType *VT = dyn_cast<VectorType>(SrcTy))
-      if (VT->getNumElements() != cast<VectorType>(DstTy)->getNumElements())
-        return false;
+    if (SrcEC != DstEC)
     return SrcTy->isIntOrIntVectorTy() && DstTy->isPtrOrPtrVectorTy();
   case Instruction::BitCast: {
     PointerType *SrcPtrTy = dyn_cast<PointerType>(SrcTy->getScalarType());
     PointerType *DstPtrTy = dyn_cast<PointerType>(DstTy->getScalarType());
 
     // BitCast implies a no-op cast of type only. No bits change.
     // However, you can't cast pointers to anything but pointers.
     if (!SrcPtrTy != !DstPtrTy)
       return false;
 
     // For non-pointer cases, the cast is okay if the source and destination bit
     // widths are identical.
     if (!SrcPtrTy)
       return SrcTy->getPrimitiveSizeInBits() == DstTy->getPrimitiveSizeInBits();
 
     // If both are pointers then the address spaces must match.
     if (SrcPtrTy->getAddressSpace() != DstPtrTy->getAddressSpace())
       return false;
 
     // A vector of pointers must have the same number of elements.
-    VectorType *SrcVecTy = dyn_cast<VectorType>(SrcTy);
-    VectorType *DstVecTy = dyn_cast<VectorType>(DstTy);
-    if (SrcVecTy && DstVecTy)
-      return (SrcVecTy->getNumElements() == DstVecTy->getNumElements());
-    if (SrcVecTy)
-      return SrcVecTy->getNumElements() == 1;
-    if (DstVecTy)
-      return DstVecTy->getNumElements() == 1;
+    if (SrcIsVec && DstIsVec)
+      return SrcEC == DstEC;
+    if (SrcIsVec)
+      return SrcEC == ElementCount(1, false);
+    if (DstIsVec)
+      return DstEC == ElementCount(1, false);
 
     return true;
   }
   case Instruction::AddrSpaceCast: {
     PointerType *SrcPtrTy = dyn_cast<PointerType>(SrcTy->getScalarType());
     if (!SrcPtrTy)
       return false;
 
     PointerType *DstPtrTy = dyn_cast<PointerType>(DstTy->getScalarType());
     if (!DstPtrTy)
       return false;
 
     if (SrcPtrTy->getAddressSpace() == DstPtrTy->getAddressSpace())
       return false;
 
-    if (VectorType *SrcVecTy = dyn_cast<VectorType>(SrcTy)) {
-      if (VectorType *DstVecTy = dyn_cast<VectorType>(DstTy))
-        return (SrcVecTy->getNumElements() == DstVecTy->getNumElements());
-
-      return false;
-    }
-
-    return true;
+    return SrcEC == DstEC;
   }
   }
 }
 
 TruncInst::TruncInst(
   Value *S, Type *Ty, const Twine &Name, Instruction *InsertBefore
 ) : CastInst(Ty, Trunc, S, Name, InsertBefore) {
   assert(castIsValid(getOpcode(), S, Ty) && "Illegal Trunc");

llvm/unittests/IR/InstructionsTest.cpp

TEST(InstructionsTest, CastInst) {
 
   Type *HalfTy = Type::getHalfTy(C);
   Type *FloatTy = Type::getFloatTy(C);
   Type *DoubleTy = Type::getDoubleTy(C);
 
   Type *V2Int32Ty = VectorType::get(Int32Ty, 2);
   Type *V2Int64Ty = VectorType::get(Int64Ty, 2);
   Type *V4Int16Ty = VectorType::get(Int16Ty, 4);
+  Type *V1Int16Ty = VectorType::get(Int16Ty, 1);
+
+  Type *VScaleV2Int32Ty = VectorType::get(Int32Ty, 2, true);
+  Type *VScaleV2Int64Ty = VectorType::get(Int64Ty, 2, true);
+  Type *VScaleV4Int16Ty = VectorType::get(Int16Ty, 4, true);
+  Type *VScaleV1Int16Ty = VectorType::get(Int16Ty, 1, true);
 
   Type *Int32PtrTy = PointerType::get(Int32Ty, 0);
   Type *Int64PtrTy = PointerType::get(Int64Ty, 0);
 
   Type *Int32PtrAS1Ty = PointerType::get(Int32Ty, 1);
   Type *Int64PtrAS1Ty = PointerType::get(Int64Ty, 1);
 
   Type *V2Int32PtrAS1Ty = VectorType::get(Int32PtrAS1Ty, 2);
   Type *V2Int64PtrAS1Ty = VectorType::get(Int64PtrAS1Ty, 2);
   Type *V4Int32PtrAS1Ty = VectorType::get(Int32PtrAS1Ty, 4);
+  Type *VScaleV4Int32PtrAS1Ty = VectorType::get(Int32PtrAS1Ty, 4, true);
   Type *V4Int64PtrAS1Ty = VectorType::get(Int64PtrAS1Ty, 4);
 
   Type *V2Int64PtrTy = VectorType::get(Int64PtrTy, 2);
   Type *V2Int32PtrTy = VectorType::get(Int32PtrTy, 2);
+  Type *VScaleV2Int32PtrTy = VectorType::get(Int32PtrTy, 2, true);
   Type *V4Int32PtrTy = VectorType::get(Int32PtrTy, 4);
+  Type *VScaleV4Int32PtrTy = VectorType::get(Int32PtrTy, 4, true);
+  Type *VScaleV4Int64PtrTy = VectorType::get(Int64PtrTy, 4, true);
 
   const Constant* c8 = Constant::getNullValue(V8x8Ty);
   const Constant* c64 = Constant::getNullValue(V8x64Ty);
 
   const Constant *v2ptr32 = Constant::getNullValue(V2Int32PtrTy);
 
   EXPECT_TRUE(CastInst::isCastable(V8x8Ty, X86MMXTy));
   EXPECT_TRUE(CastInst::isCastable(X86MMXTy, V8x8Ty));
TEST(InstructionsTest, CastInst) {
 
   EXPECT_FALSE(CastInst::castIsValid(Instruction::AddrSpaceCast,
                                      Constant::getNullValue(V4Int32PtrAS1Ty),
                                      V2Int32PtrTy));
   EXPECT_FALSE(CastInst::castIsValid(Instruction::AddrSpaceCast,
                                      Constant::getNullValue(V2Int32PtrTy),
                                      V4Int32PtrAS1Ty));
 
+  // Address space cast of fixed/scalable vectors of pointers to scalable/fixed
+  // vector of pointers.
+  EXPECT_FALSE(CastInst::castIsValid(
+      Instruction::AddrSpaceCast, Constant::getNullValue(VScaleV4Int32PtrAS1Ty),
+      V4Int32PtrTy));
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::AddrSpaceCast,
+                                     Constant::getNullValue(V4Int32PtrTy),
+                                     VScaleV4Int32PtrAS1Ty));
+  // Address space cast of scalable vectors of pointers to scalable vector of
+  // pointers.
+  EXPECT_FALSE(CastInst::castIsValid(
+      Instruction::AddrSpaceCast, Constant::getNullValue(VScaleV4Int32PtrAS1Ty),
+      VScaleV2Int32PtrTy));
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::AddrSpaceCast,
+                                     Constant::getNullValue(VScaleV2Int32PtrTy),
+                                     VScaleV4Int32PtrAS1Ty));
+  EXPECT_TRUE(CastInst::castIsValid(Instruction::AddrSpaceCast,
+                                    Constant::getNullValue(VScaleV4Int64PtrTy),
+                                    VScaleV4Int32PtrAS1Ty));
+  // Same number of lanes, different address space.
+  EXPECT_TRUE(CastInst::castIsValid(
+      Instruction::AddrSpaceCast, Constant::getNullValue(VScaleV4Int32PtrAS1Ty),
+      VScaleV4Int32PtrTy));
+  // Same number of lanes, same address space.
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::AddrSpaceCast,
+                                     Constant::getNullValue(VScaleV4Int64PtrTy),
+                                     VScaleV4Int32PtrTy));
+
+  // Bit casting fixed/scalable vector to scalable/fixed vectors.
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::BitCast,
+                                     Constant::getNullValue(V2Int32Ty),
+                                     VScaleV2Int32Ty));
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::BitCast,
+                                     Constant::getNullValue(V2Int64Ty),
+                                     VScaleV2Int64Ty));
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::BitCast,
+                                     Constant::getNullValue(V4Int16Ty),
+                                     VScaleV4Int16Ty));
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::BitCast,
+                                     Constant::getNullValue(VScaleV2Int32Ty),
+                                     V2Int32Ty));
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::BitCast,
+                                     Constant::getNullValue(VScaleV2Int64Ty),
+                                     V2Int64Ty));
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::BitCast,
+                                     Constant::getNullValue(VScaleV4Int16Ty),
+                                     V4Int16Ty));
+
+  // Bit casting scalable vectors to scalable vectors.
+  EXPECT_TRUE(CastInst::castIsValid(Instruction::BitCast,
+                                    Constant::getNullValue(VScaleV4Int16Ty),
+                                    VScaleV2Int32Ty));
+  EXPECT_TRUE(CastInst::castIsValid(Instruction::BitCast,
+                                    Constant::getNullValue(VScaleV2Int32Ty),
+                                    VScaleV4Int16Ty));
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::BitCast,
+                                     Constant::getNullValue(VScaleV2Int64Ty),
+                                     VScaleV2Int32Ty));
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::BitCast,
+                                     Constant::getNullValue(VScaleV2Int32Ty),
+                                     VScaleV2Int64Ty));
+
+  // Bitcasting to/from <vscale x 1 x Ty>
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::BitCast,
+                                     Constant::getNullValue(VScaleV1Int16Ty),
+                                     V1Int16Ty));
+  EXPECT_FALSE(CastInst::castIsValid(Instruction::BitCast,
+                                     Constant::getNullValue(V1Int16Ty),
+                                     VScaleV1Int16Ty));
 
   // Check that assertion is not hit when creating a cast with a vector of
   // pointers
   // First form
   BasicBlock *BB = BasicBlock::Create(C);
   Constant *NullV2I32Ptr = Constant::getNullValue(V2Int32PtrTy);
   auto Inst1 = CastInst::CreatePointerCast(NullV2I32Ptr, V2Int32Ty, "foo", BB);