Add CreatePointerBitCastOrAddrSpaceCast to IRBuilder and co.

[arsenm]

Summary

This already exists in ConstantExpr, but the corresponding version everywhere else is missing.

[arsenm]

Ping

[arsenm]

Ping

[arsenm]

Ping

[qcolombet]

Hi Matt,

Just to be sure, CreatePointerBitCastOrAddrSpaceCast and co are a subset of that is currently available through CreatePointerCast, right?
Apart from compile time improvement (if any) when we will use it, is there anything else we could expect from adding this API?

My point is do we really need a new API that may be confusing?
I haven’t find the discussion on why it was a good idea to add getPointerBitCastOrAddrSpaceCast for the constants in the first place, so I may just missing some context.

Sorry if I miss something obvious.

Thanks,
-Quentin

include/llvm/IR/IRBuilder.h

1191

No brackets for 1 line blocks.

[arsenm]

The reasoning is that it is more restrictive than CreatePointerCast. CreatePointerCast will create a bitcast or addrspacecast depending on the source and destination address spaces. I've found that 90+% of the time, creating an addrspacecast is a bug from not using the correct address space for one of the operands. This makes it explicit that a different address space is OK in the relatively rare places where it is expected.

[qcolombet]

So if I understood correctly, this is syntactic sugar to help spotting the places where it was likely we were not expected an addrspace to be created.
However, CreatePointerCast producing an addrspace is still a valid option.

If you believe there is a value in making such a syntactic distinction, I suggest we do a bit of a refactoring in that patch.
Let us make CreatePointerCast and co. calls CreatePointerBitCastOrAddrSpace and co.
That way, we avoid the code duplication and we still have a way to make this distinction.

What do you think?

Thanks,
-Quentin

[arsenm]

It's also more restrictive in that it doesn't allow ptrtoints. This has the same problem that I mentioned in D2205 when I added the constant version. The ptrtoint handling in CreatePointerCast would make using this in CreatePointerCast messy.

[qcolombet]

Let me be more specific on the proposal.

I think CastInst::CreatePointerCast can call CastInst::CreatePointerBitCastOrAddrSpaceCast as soon as we handled the PtrToInt part.
I.e.,
CastInst *CastInst::CreatePointerCast(Value *S, Type *Ty,

                                    const Twine &Name,
                                    BasicBlock *InsertAtEnd) {
assert(S->getType()->isPtrOrPtrVectorTy() && "Invalid cast");
assert((Ty->isIntOrIntVectorTy() || Ty->isPtrOrPtrVectorTy()) &&
       "Invalid cast");
assert(Ty->isVectorTy() == S->getType()->isVectorTy() && "Invalid cast");
assert((!Ty->isVectorTy() ||
        Ty->getVectorNumElements() == S->getType()->getVectorNumElements()) &&
       "Invalid cast");

if (Ty->isIntOrIntVectorTy())
  return Create(Instruction::PtrToInt, S, Ty, Name, InsertAtEnd);

/* ==== Call Your New method instead ==== */

Type *STy = S->getType();
if (STy->getPointerAddressSpace() != Ty->getPointerAddressSpace())
  return Create(Instruction::AddrSpaceCast, S, Ty, Name, InsertAtEnd);

return Create(Instruction::BitCast, S, Ty, Name, InsertAtEnd);

/* ====== */
}

Same thing for the instruction version.

Moreover, we should do the same for Constant *ConstantExpr::getPointerCast(Constant *S, Type *Ty).

Again maybe I am missing something, but at first glance, it seems at those points in the code path of XXXPointerCast we have the same restriction, isn’t it?

Thanks,
-Quentin

[arsenm]

Make CreatePointerCast use CreatePointerBitCastOrAddrSpaceCast

[arsenm]

Ping

[rnk]

lgtm

[arsenm]

r212962

include/llvm/Analysis/TargetFolder.h

Constant *CreateSExtOrBitCast(Constant *C, Type *DestTy) const {
     return Fold(ConstantExpr::getSExtOrBitCast(C, DestTy));
   }
   Constant *CreateTruncOrBitCast(Constant *C, Type *DestTy) const {
     if (C->getType() == DestTy)
       return C; // avoid calling Fold
     return Fold(ConstantExpr::getTruncOrBitCast(C, DestTy));
   }
 
+  Constant *CreatePointerBitCastOrAddrSpaceCast(Constant *C,
+                                                Type *DestTy) const {
+    if (C->getType() == DestTy)
+      return C; // avoid calling Fold
+    return Fold(ConstantExpr::getPointerBitCastOrAddrSpaceCast(C, DestTy));
+  }
+
   //===--------------------------------------------------------------------===//
   // Compare Instructions
   //===--------------------------------------------------------------------===//
 
   Constant *CreateICmp(CmpInst::Predicate P, Constant *LHS,
                        Constant *RHS) const {
     return Fold(ConstantExpr::getCompare(P, LHS, RHS));
   }

include/llvm/IR/ConstantFolder.h

public:
 
   Constant *CreateCast(Instruction::CastOps Op, Constant *C,
                        Type *DestTy) const {
     return ConstantExpr::getCast(Op, C, DestTy);
   }
   Constant *CreatePointerCast(Constant *C, Type *DestTy) const {
     return ConstantExpr::getPointerCast(C, DestTy);
   }
+
+  Constant *CreatePointerBitCastOrAddrSpaceCast(Constant *C,
+                                                Type *DestTy) const {
+    return ConstantExpr::getPointerBitCastOrAddrSpaceCast(C, DestTy);
+  }
+
   Constant *CreateIntCast(Constant *C, Type *DestTy,
                           bool isSigned) const {
     return ConstantExpr::getIntegerCast(C, DestTy, isSigned);
   }
   Constant *CreateFPCast(Constant *C, Type *DestTy) const {
     return ConstantExpr::getFPCast(C, DestTy);
   }
 

include/llvm/IR/IRBuilder.h

Value *CreateTruncOrBitCast(Value *V, Type *DestTy,
     return Insert(CastInst::CreateTruncOrBitCast(V, DestTy), Name);
   }
   Value *CreateCast(Instruction::CastOps Op, Value *V, Type *DestTy,
                     const Twine &Name = "") {
     if (V->getType() == DestTy)
       return V;
     if (Constant *VC = dyn_cast<Constant>(V))
       return Insert(Folder.CreateCast(Op, VC, DestTy), Name);
     return Insert(CastInst::Create(Op, V, DestTy), Name);
   }
   Value *CreatePointerCast(Value *V, Type *DestTy,
                            const Twine &Name = "") {
     if (V->getType() == DestTy)
       return V;
     if (Constant *VC = dyn_cast<Constant>(V))
       return Insert(Folder.CreatePointerCast(VC, DestTy), Name);
     return Insert(CastInst::CreatePointerCast(V, DestTy), Name);
   }
+
+  Value *CreatePointerBitCastOrAddrSpaceCast(Value *V, Type *DestTy,
+                                             const Twine &Name = "") {
+    if (V->getType() == DestTy)
+      return V;
+
+    if (Constant *VC = dyn_cast<Constant>(V)) {
+      return Insert(Folder.CreatePointerBitCastOrAddrSpaceCast(VC, DestTy),
+                    Name);
+    }
+
+    return Insert(CastInst::CreatePointerBitCastOrAddrSpaceCast(V, DestTy),
+                  Name);
+  }
+
   Value *CreateIntCast(Value *V, Type *DestTy, bool isSigned,
                        const Twine &Name = "") {
     if (V->getType() == DestTy)
       return V;
     if (Constant *VC = dyn_cast<Constant>(V))
       return Insert(Folder.CreateIntCast(VC, DestTy, isSigned), Name);
     return Insert(CastInst::CreateIntegerCast(V, DestTy, isSigned), Name);
   }

include/llvm/IR/InstrTypes.h

public:
   /// @brief Create a BitCast, AddrSpaceCast or a PtrToInt cast instruction.
   static CastInst *CreatePointerCast(
     Value *S,                ///< The pointer value to be casted (operand 0)
     Type *Ty,          ///< The type to which cast should be made
     const Twine &Name = "", ///< Name for the instruction
     Instruction *InsertBefore = nullptr ///< Place to insert the instruction
   );
 
+  /// @brief Create a BitCast or an AddrSpaceCast cast instruction.
+  static CastInst *CreatePointerBitCastOrAddrSpaceCast(
+    Value *S,                ///< The pointer 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
+  );
+
+  /// @brief Create a BitCast or an AddrSpaceCast cast instruction.
+  static CastInst *CreatePointerBitCastOrAddrSpaceCast(
+    Value *S,                ///< The pointer value to be casted (operand 0)
+    Type *Ty,          ///< The type to which cast should be made
+    const Twine &Name = "", ///< Name for the instruction
+    Instruction *InsertBefore = 0 ///< Place to insert the instruction
+  );
+
   /// @brief Create a ZExt, BitCast, or Trunc for int -> int casts.
   static CastInst *CreateIntegerCast(
     Value *S,                ///< The pointer value to be casted (operand 0)
     Type *Ty,          ///< The type to which cast should be made
     bool isSigned,           ///< Whether to regard S as signed or not
     const Twine &Name = "", ///< Name for the instruction
     Instruction *InsertBefore = nullptr ///< Place to insert the instruction
   );

lib/IR/Instructions.cpp

CastInst *CastInst::CreatePointerCast(Value *S, Type *Ty,
   assert(Ty->isVectorTy() == S->getType()->isVectorTy() && "Invalid cast");
   assert((!Ty->isVectorTy() ||
           Ty->getVectorNumElements() == S->getType()->getVectorNumElements()) &&
          "Invalid cast");
 
   if (Ty->isIntOrIntVectorTy())
     return Create(Instruction::PtrToInt, S, Ty, Name, InsertAtEnd);
 
-  Type *STy = S->getType();
-  if (STy->getPointerAddressSpace() != Ty->getPointerAddressSpace())
-    return Create(Instruction::AddrSpaceCast, S, Ty, Name, InsertAtEnd);
-
-  return Create(Instruction::BitCast, S, Ty, Name, InsertAtEnd);
+  return CreatePointerBitCastOrAddrSpaceCast(S, Ty, Name, InsertAtEnd);
 }
 
 /// @brief Create a BitCast or a PtrToInt cast instruction
 CastInst *CastInst::CreatePointerCast(Value *S, Type *Ty,
                                       const Twine &Name,
                                       Instruction *InsertBefore) {
   assert(S->getType()->isPtrOrPtrVectorTy() && "Invalid cast");
   assert((Ty->isIntOrIntVectorTy() || Ty->isPtrOrPtrVectorTy()) &&
          "Invalid cast");
   assert(Ty->isVectorTy() == S->getType()->isVectorTy() && "Invalid cast");
   assert((!Ty->isVectorTy() ||
           Ty->getVectorNumElements() == S->getType()->getVectorNumElements()) &&
          "Invalid cast");
 
   if (Ty->isIntOrIntVectorTy())
     return Create(Instruction::PtrToInt, S, Ty, Name, InsertBefore);
 
-  Type *STy = S->getType();
-  if (STy->getPointerAddressSpace() != Ty->getPointerAddressSpace())
+  return CreatePointerBitCastOrAddrSpaceCast(S, Ty, Name, InsertBefore);
+}
+
+CastInst *CastInst::CreatePointerBitCastOrAddrSpaceCast(
+  Value *S, Type *Ty,
+  const Twine &Name,
+  BasicBlock *InsertAtEnd) {
+  assert(S->getType()->isPtrOrPtrVectorTy() && "Invalid cast");
+  assert(Ty->isPtrOrPtrVectorTy() && "Invalid cast");
+
+  if (S->getType()->getPointerAddressSpace() != Ty->getPointerAddressSpace())
+    return Create(Instruction::AddrSpaceCast, S, Ty, Name, InsertAtEnd);
+
+  return Create(Instruction::BitCast, S, Ty, Name, InsertAtEnd);
+}
+
+CastInst *CastInst::CreatePointerBitCastOrAddrSpaceCast(
+  Value *S, Type *Ty,
+  const Twine &Name,
+  Instruction *InsertBefore) {
+  assert(S->getType()->isPtrOrPtrVectorTy() && "Invalid cast");
+  assert(Ty->isPtrOrPtrVectorTy() && "Invalid cast");
+
+  if (S->getType()->getPointerAddressSpace() != Ty->getPointerAddressSpace())
     return Create(Instruction::AddrSpaceCast, S, Ty, Name, InsertBefore);
 
   return Create(Instruction::BitCast, S, Ty, Name, InsertBefore);
 }
 
-CastInst *CastInst::CreateIntegerCast(Value *C, Type *Ty, 
+CastInst *CastInst::CreateIntegerCast(Value *C, Type *Ty,
                                       bool isSigned, const Twine &Name,
                                       Instruction *InsertBefore) {
   assert(C->getType()->isIntOrIntVectorTy() && Ty->isIntOrIntVectorTy() &&
          "Invalid integer cast");
   unsigned SrcBits = C->getType()->getScalarSizeInBits();
   unsigned DstBits = Ty->getScalarSizeInBits();
   Instruction::CastOps opcode =
     (SrcBits == DstBits ? Instruction::BitCast :