Fix assert with copy from global through addrspacecast

[michele.scandale]

lib/IR/Constants.cpp

1510

Maybe this function can be fused within getPointerCast in order to support there the case of pointers with different address spaces.

[arsenm]

lib/IR/Constants.cpp

1510

I tried that originally, but the results weren't satisfying. The asserts are different since getBitCastOrAddrSpaceCast only accepts pointer<->pointer casts, while getPointerCast accepts integer destinations.

[michele.scandale]

lib/IR/Constants.cpp

1510

That's ok, IMHO a more general name would be better (like getPointerToPointerCast).

However, the getPointerCast must be fixed in order to create the proper cast also when the destination type has a different address space with respect to the source Constant.

[arsenm]

lib/IR/Constants.cpp

1510

That is already handled, I just updated the comment

[michele.scandale]

LGTM!

[michele.scandale]

lib/IR/Constants.cpp

1510

I think that the semantic for 'getBitCastOrAddrSpaceCast' should be to pick a bitcast wheter is valid or an addrspace cast whether is valid or assert.
A set of functions like this, also for instructions, and a wrapper in the IRBuilder will be very useful to simplify the upgrade of the frontend and other tools to the new bitcast semantic and the new instruction (e.g. where previously there was a CreateBitCast now a CreateBitCastOrAddrSpaceCast would be fine).

What do you think about this?

[arsenm]

I think it's better for it to be stricter and only work for pointers. A better name might be getPointerBitCastOrAddrSpaceCast? IRBuilder equivalents of whatever is there is a good idea.

[michele.scandale]

The suggested name won't be ambiguous, so +1.

BTW, I don't see the problem, if a non pointer value is passed, an addrspacecast would be illegal because it requires a pointer value, while a bitcast would be valid w.r.t. its constraints.

Constant *ConstantExpr::getBitCastOrAddrSpaceCast(Constant *S, Type *Ty) {       
  assert((CastInst::castIsValid(Instruction::BitCast, S, Ty) ||                  
          CastInst::castIsValid(Instruction::AddrSpaceCast, S, Ty)) &&           
          "Invalid cast");                                                       
                                                                               
  unsigned SrcTy = S->getType();                                                 
  if (Ty->isPtrOrPtrVectorTy() && DestTy->isPtrOrPtrVectorTy() &&                
      SrcTy->getPointerAddressSpace() != Ty->getPointerAddressSpace())           
    return getAddrSpaceCast(S, Ty);                                              
                                                                               
  return getBitCast(S, Ty);                                                      
}

But maybe something like this could just an helper in IRBuilder...

[arsenm]

The problem is passing in something that isn't a pointer that you didn't mean to. Generally doing whatever it takes to convert types isn't desirable. If you're expecting to get a pointer-only cast like addrspacecast, you probably don't want an integer to be passed in.

[arsenm]

Also, not accepting integers is also the distinguishing feature from getPointerCast

[michele.scandale]

That's fine, with the name 'getPointerBitCastOrAddrSpaceCast' it will avoid ambiguities.

[arsenm]

r196638

include/llvm/IR/Constants.h

     Type *Ty ///< The type to trunc or bitcast C to
   );
 
-  /// @brief Create a BitCast or a PtrToInt cast constant expression
+  /// @brief Create a BitCast, AddrSpaceCast, or a PtrToInt cast constant
+  /// expression.
   static Constant *getPointerCast(
     Constant *C,   ///< The pointer value to be casted (operand 0)
     Type *Ty ///< The type to which cast should be made
   );
 
+  /// @brief Create a BitCast or AddrSpaceCast depending on the address space.
+  static Constant *getBitCastOrAddrSpaceCast(
+    Constant *C,   ///< The constant to addrspacecast or bitcast
+    Type *Ty ///< The type to bitcast or addrspacecast C to
+  );
+
   /// @brief Create a ZExt, Bitcast or Trunc for integer -> integer casts
   static Constant *getIntegerCast(
     Constant *C,    ///< The integer constant to be casted

lib/IR/Constants.cpp

   return getBitCast(S, Ty);
 }
 
-Constant *ConstantExpr::getIntegerCast(Constant *C, Type *Ty, 
+Constant *ConstantExpr::getBitCastOrAddrSpaceCast(Constant *S, Type *Ty) {
+  assert(S->getType()->isPtrOrPtrVectorTy() && "Invalid cast");
+  assert(Ty->isPtrOrPtrVectorTy() && "Invalid cast");
+
+  if (S->getType()->getPointerAddressSpace() != Ty->getPointerAddressSpace())
+    return getAddrSpaceCast(S, Ty);
+
+  return getBitCast(S, Ty);
+}
+
+Constant *ConstantExpr::getIntegerCast(Constant *C, Type *Ty,
                                        bool isSigned) {
   assert(C->getType()->isIntOrIntVectorTy() &&
          Ty->isIntOrIntVectorTy() && "Invalid cast");

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

         for (unsigned i = 0, e = ToDelete.size(); i != e; ++i)
           EraseInstFromFunction(*ToDelete[i]);
         Constant *TheSrc = cast<Constant>(Copy->getSource());
-        Instruction *NewI
-          = ReplaceInstUsesWith(AI, ConstantExpr::getBitCast(TheSrc,
-                                                             AI.getType()));
+        Constant *Cast
+          = ConstantExpr::getBitCastOrAddrSpaceCast(TheSrc, AI.getType());
+        Instruction *NewI = ReplaceInstUsesWith(AI, Cast);
         EraseInstFromFunction(*Copy);
         ++NumGlobalCopies;
         return NewI;

test/Transforms/InstCombine/addrspacecast.ll

 
 target datalayout = "e-p:64:64:64-p1:32:32:32-p2:16:16:16-n8:16:32:64"
 
+
+declare void @llvm.memcpy.p0i8.p0i8.i32(i8*, i8*, i32, i32, i1) nounwind
+declare void @llvm.memcpy.p0i8.p1i8.i32(i8*, i8 addrspace(1)*, i32, i32, i1) nounwind
+declare void @llvm.memcpy.p0i8.p2i8.i32(i8*, i8 addrspace(2)*, i32, i32, i1) nounwind
+
+
 define i32* @combine_redundant_addrspacecast(i32 addrspace(1)* %x) nounwind {
 ; CHECK-LABEL: @combine_redundant_addrspacecast(
 ; CHECK: addrspacecast i32 addrspace(1)* %x to i32*

   ret float* %z
 }
 
+@const_array = addrspace(2) constant [60 x i8] [i8 2, i8 9, i8 4, i8 22, i8 2, i8 9, i8 4, i8 22, i8 2, i8 9, i8 4, i8 22,
+                                                i8 2, i8 9, i8 4, i8 22, i8 2, i8 9, i8 4, i8 22, i8 2, i8 9, i8 4, i8 22,
+                                                i8 2, i8 9, i8 4, i8 22, i8 2, i8 9, i8 4, i8 22, i8 2, i8 9, i8 4, i8 22,
+                                                i8 2, i8 9, i8 4, i8 22, i8 2, i8 9, i8 4, i8 22, i8 2, i8 9, i8 4, i8 22,
+                                                i8 2, i8 9, i8 4, i8 22, i8 2, i8 9, i8 4, i8 22, i8 2, i8 9, i8 4, i8 22 ]
+
+declare void @foo(i8*) nounwind
+
+; A copy from a constant addrspacecast'ed global
+; CHECK-LABEL: @memcpy_addrspacecast(
+; CHECK-NOT:  call void @llvm.memcpy
+define i32 @memcpy_addrspacecast() nounwind {
+entry:
+  %alloca = alloca i8, i32 48
+  call void @llvm.memcpy.p0i8.p1i8.i32(i8* %alloca, i8 addrspace(1)* addrspacecast (i8 addrspace(2)* getelementptr inbounds ([60 x i8] addrspace(2)* @const_array, i16 0, i16 4) to i8 addrspace(1)*), i32 48, i32 4, i1 false) nounwind
+  br label %loop.body
+
+loop.body:
+  %i = phi i32 [ 0, %entry ], [ %i.inc, %loop.body ]
+  %sum = phi i32 [ 0, %entry ], [ %sum.inc, %loop.body]
+  %ptr = getelementptr i8* %alloca, i32 %i
+  %load = load i8* %ptr
+  %ext = zext i8 %load to i32
+  %sum.inc = add i32 %sum, %ext
+  %i.inc = add i32 %i, 1
+  %cmp = icmp ne i32 %i, 48
+  br i1 %cmp, label %loop.body, label %end
+
+end:
+  ret i32 %sum.inc
+}
+