[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

3242

What's wrong with getScalarSizeInBits()?

3282

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

[fpetrogalli]

llvm/lib/IR/Instructions.cpp

3241–3242

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?

3242

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

3282

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

3241–3242

Sure.

3242

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

3282

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

3279

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

3336

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();
+  TypeSize SrcBitSize = SrcTy->getPrimitiveSizeInBits();
+  TypeSize DstBitSize = DstTy->getPrimitiveSizeInBits();
 
   // 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;
CastInst::castIsValid(Instruction::CastOps op, Value *S, Type *DstTy) {
   case Instruction::SIToFP:
     return SrcTy->isIntOrIntVectorTy() && DstTy->isFPOrFPVectorTy() &&
       SrcLength == DstLength;
   case Instruction::FPToUI:
   case Instruction::FPToSI:
     return SrcTy->isFPOrFPVectorTy() && DstTy->isIntOrIntVectorTy() &&
       SrcLength == DstLength;
   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;
     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;
case Instruction::AddrSpaceCast: {
     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;
   }
   }
 }