[IR] Remove CastInst::isCastable since it is not used

[c-rhodes]

Summary

It was removed back in 2013 (f63dfbb) by Matt Arsenault but then
reverted since DragonEgg used it, but that project is no longer
maintained.

Can this be removed now?

[ldionne]

The libc++abi part is fine.

[dexonsmith]

libcxxabi/test/test_demangle.pass.cpp

21547 ↗

(On Diff #309227)

I don't understand why this is being removed. Is the demangler testcase no longer valid?

[ldionne]

libcxxabi/test/test_demangle.pass.cpp

21547 ↗

(On Diff #309227)

It's still valid -- the only reason why I'm not opposing to the removal is that I can see a *small* benefit in not leaving behind old definitions like that, which show up whenever you grep for the function name in the whole monorepo.

Basically, I don't mind whether we keep it or not. I think the benefit is small, but the loss of test coverage is also very small, since I assume those symbols were just taken more or less randomly and passed through c++filt.

[dexonsmith]

libcxxabi/test/test_demangle.pass.cpp

21547 ↗

(On Diff #309227)

Yeah, I would have to guess that all the symbols in an LLVM binary were demangled with our previous system demangler, to add as testcases for libc++abi.

My inclination is that it's better to leave it behind, until / unless the demangler tests are rethought as a whole in some way (to get similar coverage another way).

I agree that this file coming up in git grep is a bit annoying (it happens a lot to me)... but since this patch deletes CastInst::isCastable I expect this line won't bother anyone again (since no one will be looking for the deleted function). It's all the other lines that will continue to cause friction.

I also worry that if we start deleting this one at a time, we'll add a bunch of uninteresting commits to:

% git log -- libcxxabi/

WDYT?

[ldionne]

libcxxabi/test/test_demangle.pass.cpp

21547 ↗

(On Diff #309227)

Ok, I'm fine with your suggestion. @c-rhodes , can you please leave that test in libc++abi? And then libc++abi won't be affected by this change, so you still have my LGTM :-).

[dexonsmith]

LGTM too if you drop the libcxxabi change.

If there's an out-of-tree user that needs this we can revert again if it makes sense to (in which case, it'd be better to change getCastOpcode to return an Optional<Instruction::CastOps>, so we're not having to keep two functions in sync like this...).

[c-rhodes]

Remove change to libcxxabi test.

[c-rhodes]

In D92571#2431860, @dexonsmith wrote:

LGTM too if you drop the libcxxabi change.

If there's an out-of-tree user that needs this we can revert again if it makes sense to (in which case, it'd be better to change getCastOpcode to return an Optional<Instruction::CastOps>, so we're not having to keep two functions in sync like this...).

SGTM, I'll land it early next week in-case of any fallout, thanks for reviewing.

libcxxabi/test/test_demangle.pass.cpp

21547 ↗

(On Diff #309227)

Ok, I'm fine with your suggestion. @c-rhodes , can you please leave that test in libc++abi? And then libc++abi won't be affected by this change, so you still have my LGTM :-).

np cheers, I've removed the change.

llvm/include/llvm/IR/InstrTypes.h

public:
   /// Create a Trunc or BitCast cast instruction
   static CastInst *CreateTruncOrBitCast(
     Value *S,                ///< The value to be casted (operand 0)
     Type *Ty,          ///< The type to which operand is casted
     const Twine &Name, ///< The name for the instruction
     BasicBlock *InsertAtEnd  ///< The block to insert the instruction into
   );
 
-  /// Check whether it is valid to call getCastOpcode for these types.
-  static bool isCastable(
-    Type *SrcTy, ///< The Type from which the value should be cast.
-    Type *DestTy ///< The Type to which the value should be cast.
-  );
-
   /// Check whether a bitcast between these types is valid
   static bool isBitCastable(
     Type *SrcTy, ///< The Type from which the value should be cast.
     Type *DestTy ///< The Type to which the value should be cast.
   );
 
   /// Check whether a bitcast, inttoptr, or ptrtoint cast between these
   /// types is valid and a no-op.

llvm/lib/IR/Instructions.cpp

CastInst *CastInst::CreateFPCast(Value *C, Type *Ty,
   unsigned SrcBits = C->getType()->getScalarSizeInBits();
   unsigned DstBits = Ty->getScalarSizeInBits();
   Instruction::CastOps opcode =
     (SrcBits == DstBits ? Instruction::BitCast :
      (SrcBits > DstBits ? Instruction::FPTrunc : Instruction::FPExt));
   return Create(opcode, C, Ty, Name, InsertAtEnd);
 }
 
-// Check whether it is valid to call getCastOpcode for these types.
-// This routine must be kept in sync with getCastOpcode.
-bool CastInst::isCastable(Type *SrcTy, Type *DestTy) {
-  if (!SrcTy->isFirstClassType() || !DestTy->isFirstClassType())
-    return false;
-
-  if (SrcTy == DestTy)
-    return true;
-
-  if (VectorType *SrcVecTy = dyn_cast<VectorType>(SrcTy))
-    if (VectorType *DestVecTy = dyn_cast<VectorType>(DestTy))
-      if (cast<FixedVectorType>(SrcVecTy)->getNumElements() ==
-          cast<FixedVectorType>(DestVecTy)->getNumElements()) {
-        // An element by element cast.  Valid if casting the elements is valid.
-        SrcTy = SrcVecTy->getElementType();
-        DestTy = DestVecTy->getElementType();
-      }
-
-  // Get the bit sizes, we'll need these
-  TypeSize SrcBits = SrcTy->getPrimitiveSizeInBits();   // 0 for ptr
-  TypeSize DestBits = DestTy->getPrimitiveSizeInBits(); // 0 for ptr
-
-  // Run through the possibilities ...
-  if (DestTy->isIntegerTy()) {               // Casting to integral
-    if (SrcTy->isIntegerTy())                // Casting from integral
-        return true;
-    if (SrcTy->isFloatingPointTy())   // Casting from floating pt
-      return true;
-    if (SrcTy->isVectorTy())          // Casting from vector
-      return DestBits == SrcBits;
-                                      // Casting from something else
-    return SrcTy->isPointerTy();
-  }
-  if (DestTy->isFloatingPointTy()) {  // Casting to floating pt
-    if (SrcTy->isIntegerTy())                // Casting from integral
-      return true;
-    if (SrcTy->isFloatingPointTy())   // Casting from floating pt
-      return true;
-    if (SrcTy->isVectorTy())          // Casting from vector
-      return DestBits == SrcBits;
-                                    // Casting from something else
-    return false;
-  }
-  if (DestTy->isVectorTy())         // Casting to vector
-    return DestBits == SrcBits;
-  if (DestTy->isPointerTy()) {        // Casting to pointer
-    if (SrcTy->isPointerTy())                // Casting from pointer
-      return true;
-    return SrcTy->isIntegerTy();             // Casting from integral
-  }
-  if (DestTy->isX86_MMXTy()) {
-    if (SrcTy->isVectorTy())
-      return DestBits == SrcBits;       // 64-bit vector to MMX
-    return false;
-  }                                    // Casting to something else
-  return false;
-}
-
 bool CastInst::isBitCastable(Type *SrcTy, Type *DestTy) {
   if (!SrcTy->isFirstClassType() || !DestTy->isFirstClassType())
     return false;
 
   if (SrcTy == DestTy)
     return true;
 
   if (VectorType *SrcVecTy = dyn_cast<VectorType>(SrcTy)) {

 }
 
 // Provide a way to get a "cast" where the cast opcode is inferred from the
 // types and size of the operand. This, basically, is a parallel of the
 // logic in the castIsValid function below.  This axiom should hold:
 //   castIsValid( getCastOpcode(Val, Ty), Val, Ty)
 // should not assert in castIsValid. In other words, this produces a "correct"
 // casting opcode for the arguments passed to it.
-// This routine must be kept in sync with isCastable.
 Instruction::CastOps
 CastInst::getCastOpcode(
   const Value *Src, bool SrcIsSigned, Type *DestTy, bool DestIsSigned) {
   Type *SrcTy = Src->getType();
 
   assert(SrcTy->isFirstClassType() && DestTy->isFirstClassType() &&
          "Only first class types are castable!");
 

llvm/unittests/IR/InstructionsTest.cpp

TEST(InstructionsTest, CastInst) {
   Type *VScaleV4Int32PtrTy = ScalableVectorType::get(Int32PtrTy, 4);
   Type *VScaleV4Int64PtrTy = ScalableVectorType::get(Int64PtrTy, 4);
 
   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));
-  EXPECT_FALSE(CastInst::isCastable(Int64Ty, X86MMXTy));
-  EXPECT_TRUE(CastInst::isCastable(V8x64Ty, V8x8Ty));
-  EXPECT_TRUE(CastInst::isCastable(V8x8Ty, V8x64Ty));
   EXPECT_EQ(CastInst::Trunc, CastInst::getCastOpcode(c64, true, V8x8Ty, true));
   EXPECT_EQ(CastInst::SExt, CastInst::getCastOpcode(c8, true, V8x64Ty, true));
 
   EXPECT_FALSE(CastInst::isBitCastable(V8x8Ty, X86MMXTy));
   EXPECT_FALSE(CastInst::isBitCastable(X86MMXTy, V8x8Ty));
   EXPECT_FALSE(CastInst::isBitCastable(Int64Ty, X86MMXTy));
   EXPECT_FALSE(CastInst::isBitCastable(V8x64Ty, V8x8Ty));
   EXPECT_FALSE(CastInst::isBitCastable(V8x8Ty, V8x64Ty));
 
   // Check address space casts are rejected since we don't know the sizes here
   EXPECT_FALSE(CastInst::isBitCastable(Int32PtrTy, Int32PtrAS1Ty));
   EXPECT_FALSE(CastInst::isBitCastable(Int32PtrAS1Ty, Int32PtrTy));
   EXPECT_FALSE(CastInst::isBitCastable(V2Int32PtrTy, V2Int32PtrAS1Ty));
   EXPECT_FALSE(CastInst::isBitCastable(V2Int32PtrAS1Ty, V2Int32PtrTy));
   EXPECT_TRUE(CastInst::isBitCastable(V2Int32PtrAS1Ty, V2Int64PtrAS1Ty));
-  EXPECT_TRUE(CastInst::isCastable(V2Int32PtrAS1Ty, V2Int32PtrTy));
   EXPECT_EQ(CastInst::AddrSpaceCast, CastInst::getCastOpcode(v2ptr32, true,
                                                              V2Int32PtrAS1Ty,
                                                              true));
 
   // Test mismatched number of elements for pointers
   EXPECT_FALSE(CastInst::isBitCastable(V2Int32PtrAS1Ty, V4Int64PtrAS1Ty));
   EXPECT_FALSE(CastInst::isBitCastable(V4Int64PtrAS1Ty, V2Int32PtrAS1Ty));
   EXPECT_FALSE(CastInst::isBitCastable(V2Int32PtrAS1Ty, V4Int32PtrAS1Ty));