[OpenCL] AMDGCN: Fix size_t type

[yaxunl]

Summary

Pointers of certain GPUs in AMDGCN target in private address space is 32 bit but pointers in other address spaces are 64 bit. size_t type should be defined as 64 bit for these GPUs so that it could hold pointers in all address spaces.

[arsenm]

lib/Basic/Targets.cpp

2008–2010 ↗

(On Diff #67554)

Should use ternary operator and check the triple arch instead

[yaxunl]

Revised by Matt's comments.

[Anastasia]

I think Clang is supposed to generate the IR specific to the target architecture. It seems strange to ignore the pointer size. I am not sure if it might lead to some issues for the backends.

lib/CodeGen/CodeGenModule.cpp

107 ↗

(On Diff #67687)

It seems wrong to call OpenCL specific function in generic code path?

[yaxunl]

In D23361#512729, @Anastasia wrote:

I think Clang is supposed to generate the IR specific to the target architecture. It seems strange to ignore the pointer size. I am not sure if it might lead to some issues for the backends.

This change only affects AMDGCN target which has different pointer size for different address spaces. There is no change for other targets.

For AMDGCN target, since there can be only one definition for size_t type, it has to be 64 bit to accommodate all pointers.

For operations not involving pointers, this is fine since it does not matter whether size_t is i32 or i64.

For operations involving pointers, it only affects ptrtoint instruction, which allows a pointer to be casted to any integer. If the integer size is larger than the pointer size, it is zero extended. If the integer is smaller than the pointer size, it is truncated.

When clang casts a pointer to size_t, there are two cases:

1. non-private pointers: before this change, it was casted to i32, which is wrong. now it is casted to i64 correctly.

2. private pointer: before this change, it was casted to i32, now it is casted to i64. It is still correct. Backend should be able to optimize these instructions.

[joey]

lib/CodeGen/CodeGenModule.cpp

101 ↗

(On Diff #67687)

What if you create a new function in TargetInfo called getMaxPointerWidth(unsigned AddrSpace), and call that here? That would by default just call 'getPointerWidth', but in your AMDGPU TargetInfo you can override that.
That feels more generic.

[yaxunl]

lib/CodeGen/CodeGenModule.cpp

101 ↗

(On Diff #67687)

Thanks for your suggestion. There are some other changes needed in addition to that.

[yaxunl]

Remove language dependency. Choose proper integer types for pointer arithmetic.

[arsenm]

lib/CodeGen/CodeGenModule.h

664 ↗

(On Diff #68241)

This has the same purpose as DataLayout::getIntPtrType, should probably just use that

[yaxunl]

Fix lit test. Use DataLayout::getIntPtrType as Matt suggested.

[Anastasia]

lib/CodeGen/CGExprScalar.cpp

1513 ↗

(On Diff #68360)

Did you miss this changes in the original patch then?

2447 ↗

(On Diff #68360)

No longer ptrdiff_t?

[yaxunl]

lib/CodeGen/CGExprScalar.cpp

1513 ↗

(On Diff #68360)

Yes.

2447 ↗

(On Diff #68360)

PtrDiffTy and IntPtrTy are members of an anonymous union

/// intptr_t, size_t, and ptrdiff_t, which we assume are the same size.
union {
  llvm::IntegerType *IntPtrTy;
  llvm::IntegerType *SizeTy;
  llvm::IntegerType *PtrDiffTy;
};

so they are actually the same thing.

[Anastasia]

LGTM!

cfe/trunk/include/clang/Basic/TargetInfo.h

public:
   /// specified address space.
   uint64_t getPointerWidth(unsigned AddrSpace) const {
     return AddrSpace == 0 ? PointerWidth : getPointerWidthV(AddrSpace);
   }
   uint64_t getPointerAlign(unsigned AddrSpace) const {
     return AddrSpace == 0 ? PointerAlign : getPointerAlignV(AddrSpace);
   }
 
+  /// \brief Return the maximum width of pointers on this target.
+  virtual uint64_t getMaxPointerWidth() const {
+    return PointerWidth;
+  }
+
   /// \brief Return the size of '_Bool' and C++ 'bool' for this target, in bits.
   unsigned getBoolWidth() const { return BoolWidth; }
 
   /// \brief Return the alignment of '_Bool' and C++ 'bool' for this target.
   unsigned getBoolAlign() const { return BoolAlign; }
 
   unsigned getCharWidth() const { return 8; } // FIXME
   unsigned getCharAlign() const { return 8; } // FIXME

cfe/trunk/lib/Basic/TargetInfo.cpp

if (Opts.OpenCL) {
     // defined as float. Let's not override this as it might lead
     // to generating illegal code that uses 64bit doubles.
     if (DoubleWidth != FloatWidth) {
       DoubleWidth = DoubleAlign = 64;
       DoubleFormat = &llvm::APFloat::IEEEdouble;
     }
     LongDoubleWidth = LongDoubleAlign = 128;
 
-    assert(PointerWidth == 32 || PointerWidth == 64);
-    bool Is32BitArch = PointerWidth == 32;
+    unsigned MaxPointerWidth = getMaxPointerWidth();
+    assert(MaxPointerWidth == 32 || MaxPointerWidth == 64);
+    bool Is32BitArch = MaxPointerWidth == 32;
     SizeType = Is32BitArch ? UnsignedInt : UnsignedLong;
     PtrDiffType = Is32BitArch ? SignedInt : SignedLong;
     IntPtrType = Is32BitArch ? SignedInt : SignedLong;
 
     IntMaxType = SignedLongLong;
     Int64Type = SignedLong;
 
     HalfFormat = &llvm::APFloat::IEEEhalf;

cfe/trunk/lib/Basic/Targets.cpp

switch(AddrSpace) {
         return 64;
       case 0:
       case 3:
       case 5:
         return 32;
     }
   }
 
+  uint64_t getMaxPointerWidth() const override {
+    return getTriple().getArch() == llvm::Triple::amdgcn ? 64 : 32;
+  }
+
   const char * getClobbers() const override {
     return "";
   }
 
   ArrayRef<const char *> getGCCRegNames() const override;
 
   ArrayRef<TargetInfo::GCCRegAlias> getGCCRegAliases() const override {
     return None;

cfe/trunk/lib/CodeGen/CGExprScalar.cpp

Value *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType SrcType,
 
   // Ignore conversions like int -> uint.
   if (SrcTy == DstTy)
     return Src;
 
   // Handle pointer conversions next: pointers can only be converted to/from
   // other pointers and integers. Check for pointer types in terms of LLVM, as
   // some native types (like Obj-C id) may map to a pointer type.
-  if (isa<llvm::PointerType>(DstTy)) {
+  if (auto DstPT = dyn_cast<llvm::PointerType>(DstTy)) {
     // The source value may be an integer, or a pointer.
     if (isa<llvm::PointerType>(SrcTy))
       return Builder.CreateBitCast(Src, DstTy, "conv");
 
     assert(SrcType->isIntegerType() && "Not ptr->ptr or int->ptr conversion?");
     // First, convert to the correct width so that we control the kind of
     // extension.
-    llvm::Type *MiddleTy = CGF.IntPtrTy;
+    llvm::Type *MiddleTy = CGF.CGM.getDataLayout().getIntPtrType(DstPT);
     bool InputSigned = SrcType->isSignedIntegerOrEnumerationType();
     llvm::Value* IntResult =
         Builder.CreateIntCast(Src, MiddleTy, InputSigned, "conv");
     // Then, cast to pointer.
     return Builder.CreateIntToPtr(IntResult, DstTy, "conv");
   }
 
   if (isa<llvm::PointerType>(SrcTy)) {
case CK_LValueToRValue:
     assert(E->isGLValue() && "lvalue-to-rvalue applied to r-value!");
     return Visit(const_cast<Expr*>(E));
 
   case CK_IntegralToPointer: {
     Value *Src = Visit(const_cast<Expr*>(E));
 
     // First, convert to the correct width so that we control the kind of
     // extension.
-    llvm::Type *MiddleTy = CGF.IntPtrTy;
+    auto DestLLVMTy = ConvertType(DestTy);
+    llvm::Type *MiddleTy = CGF.CGM.getDataLayout().getIntPtrType(DestLLVMTy);
     bool InputSigned = E->getType()->isSignedIntegerOrEnumerationType();
     llvm::Value* IntResult =
       Builder.CreateIntCast(Src, MiddleTy, InputSigned, "conv");
 
-    return Builder.CreateIntToPtr(IntResult, ConvertType(DestTy));
+    return Builder.CreateIntToPtr(IntResult, DestLLVMTy);
   }
   case CK_PointerToIntegral:
     assert(!DestTy->isBooleanType() && "bool should use PointerToBool");
     return Builder.CreatePtrToInt(Visit(E), ConvertType(DestTy));
 
   case CK_ToVoid: {
     CGF.EmitIgnoredExpr(E);
     return nullptr;
static Value *emitPointerArithmetic(CodeGenFunction &CGF,
                                     const BinOpInfo &op,
                                     bool isSubtraction) {
   // Must have binary (not unary) expr here.  Unary pointer
   // increment/decrement doesn't use this path.
   const BinaryOperator *expr = cast<BinaryOperator>(op.E);
 
   Value *pointer = op.LHS;
   Expr *pointerOperand = expr->getLHS();
+  auto PtrTy = cast<llvm::PointerType>(pointer->getType());
   Value *index = op.RHS;
   Expr *indexOperand = expr->getRHS();
 
   // In a subtraction, the LHS is always the pointer.
   if (!isSubtraction && !pointer->getType()->isPointerTy()) {
     std::swap(pointer, index);
     std::swap(pointerOperand, indexOperand);
   }
 
   unsigned width = cast<llvm::IntegerType>(index->getType())->getBitWidth();
-  if (width != CGF.PointerWidthInBits) {
+  auto &DL = CGF.CGM.getDataLayout();
+  if (width != DL.getTypeSizeInBits(PtrTy)) {
     // Zero-extend or sign-extend the pointer value according to
     // whether the index is signed or not.
     bool isSigned = indexOperand->getType()->isSignedIntegerOrEnumerationType();
-    index = CGF.Builder.CreateIntCast(index, CGF.PtrDiffTy, isSigned,
+    index = CGF.Builder.CreateIntCast(index, DL.getIntPtrType(PtrTy), isSigned,
                                       "idx.ext");
   }
 
   // If this is subtraction, negate the index.
   if (isSubtraction)
     index = CGF.Builder.CreateNeg(index, "idx.neg");
 
   if (CGF.SanOpts.has(SanitizerKind::ArrayBounds))

cfe/trunk/lib/CodeGen/CodeGenModule.cpp

CodeGenModule::CodeGenModule(ASTContext &C, const HeaderSearchOptions &HSO,
   Int16Ty = llvm::Type::getInt16Ty(LLVMContext);
   Int32Ty = llvm::Type::getInt32Ty(LLVMContext);
   Int64Ty = llvm::Type::getInt64Ty(LLVMContext);
   FloatTy = llvm::Type::getFloatTy(LLVMContext);
   DoubleTy = llvm::Type::getDoubleTy(LLVMContext);
   PointerWidthInBits = C.getTargetInfo().getPointerWidth(0);
   PointerAlignInBytes =
     C.toCharUnitsFromBits(C.getTargetInfo().getPointerAlign(0)).getQuantity();
+  SizeSizeInBytes =
+    C.toCharUnitsFromBits(C.getTargetInfo().getMaxPointerWidth()).getQuantity();
   IntAlignInBytes =
     C.toCharUnitsFromBits(C.getTargetInfo().getIntAlign()).getQuantity();
   IntTy = llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getIntWidth());
-  IntPtrTy = llvm::IntegerType::get(LLVMContext, PointerWidthInBits);
+  IntPtrTy = llvm::IntegerType::get(LLVMContext,
+    C.getTargetInfo().getMaxPointerWidth());
   Int8PtrTy = Int8Ty->getPointerTo(0);
   Int8PtrPtrTy = Int8PtrTy->getPointerTo(0);
 
   RuntimeCC = getTargetCodeGenInfo().getABIInfo().getRuntimeCC();
   BuiltinCC = getTargetCodeGenInfo().getABIInfo().getBuiltinCC();
 
   if (LangOpts.ObjC1)
     createObjCRuntime();

cfe/trunk/lib/CodeGen/CodeGenTypeCache.h

struct CodeGenTypeCache {
 
   /// The width of a pointer into the generic address space.
   unsigned char PointerWidthInBits;
 
   /// The size and alignment of a pointer into the generic address space.
   union {
     unsigned char PointerAlignInBytes;
     unsigned char PointerSizeInBytes;
+  };
+
+  /// The size and alignment of size_t.
+  union {
     unsigned char SizeSizeInBytes; // sizeof(size_t)
     unsigned char SizeAlignInBytes;
   };
+
   CharUnits getSizeSize() const {
     return CharUnits::fromQuantity(SizeSizeInBytes);
   }
   CharUnits getSizeAlign() const {
     return CharUnits::fromQuantity(SizeAlignInBytes);
   }
   CharUnits getPointerSize() const {
     return CharUnits::fromQuantity(PointerSizeInBytes);

cfe/trunk/test/CodeGenOpenCL/size_t.cl

+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -emit-llvm -O0 -triple spir-unknown-unknown -o - | FileCheck --check-prefix=SZ32 %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -emit-llvm -O0 -triple spir64-unknown-unknown -o - | FileCheck --check-prefix=SZ64 --check-prefix=SZ64ONLY %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -finclude-default-header -emit-llvm -O0 -triple amdgcn-- -o - | FileCheck --check-prefix=SZ64 --check-prefix=AMDONLY %s
+
+//SZ32: define{{.*}} i32 @test_ptrtoint_private(i8* %x)
+//SZ32: ptrtoint i8* %{{.*}} to i32
+//SZ64: define{{.*}} i64 @test_ptrtoint_private(i8* %x)
+//SZ64: ptrtoint i8* %{{.*}} to i64
+size_t test_ptrtoint_private(private char* x) {
+  return (size_t)x;
+}
+
+//SZ32: define{{.*}} i32 @test_ptrtoint_global(i8 addrspace(1)* %x)
+//SZ32: ptrtoint i8 addrspace(1)* %{{.*}} to i32
+//SZ64: define{{.*}} i64 @test_ptrtoint_global(i8 addrspace(1)* %x)
+//SZ64: ptrtoint i8 addrspace(1)* %{{.*}} to i64
+intptr_t test_ptrtoint_global(global char* x) {
+  return (intptr_t)x;
+}
+
+//SZ32: define{{.*}} i32 @test_ptrtoint_constant(i8 addrspace(2)* %x)
+//SZ32: ptrtoint i8 addrspace(2)* %{{.*}} to i32
+//SZ64: define{{.*}} i64 @test_ptrtoint_constant(i8 addrspace(2)* %x)
+//SZ64: ptrtoint i8 addrspace(2)* %{{.*}} to i64
+uintptr_t test_ptrtoint_constant(constant char* x) {
+  return (uintptr_t)x;
+}
+
+//SZ32: define{{.*}} i32 @test_ptrtoint_local(i8 addrspace(3)* %x)
+//SZ32: ptrtoint i8 addrspace(3)* %{{.*}} to i32
+//SZ64: define{{.*}} i64 @test_ptrtoint_local(i8 addrspace(3)* %x)
+//SZ64: ptrtoint i8 addrspace(3)* %{{.*}} to i64
+size_t test_ptrtoint_local(local char* x) {
+  return (size_t)x;
+}
+
+//SZ32: define{{.*}} i32 @test_ptrtoint_generic(i8 addrspace(4)* %x)
+//SZ32: ptrtoint i8 addrspace(4)* %{{.*}} to i32
+//SZ64: define{{.*}} i64 @test_ptrtoint_generic(i8 addrspace(4)* %x)
+//SZ64: ptrtoint i8 addrspace(4)* %{{.*}} to i64
+size_t test_ptrtoint_generic(generic char* x) {
+  return (size_t)x;
+}
+
+//SZ32: define{{.*}} i8* @test_inttoptr_private(i32 %x)
+//SZ32: inttoptr i32 %{{.*}} to i8*
+//SZ64: define{{.*}} i8* @test_inttoptr_private(i64 %x)
+//AMDONLY: trunc i64 %{{.*}} to i32
+//AMDONLY: inttoptr i32 %{{.*}} to i8*
+//SZ64ONLY: inttoptr i64 %{{.*}} to i8*
+private char* test_inttoptr_private(size_t x) {
+  return (private char*)x;
+}
+
+//SZ32: define{{.*}} i8 addrspace(1)* @test_inttoptr_global(i32 %x)
+//SZ32: inttoptr i32 %{{.*}} to i8 addrspace(1)*
+//SZ64: define{{.*}} i8 addrspace(1)* @test_inttoptr_global(i64 %x)
+//SZ64: inttoptr i64 %{{.*}} to i8 addrspace(1)*
+global char* test_inttoptr_global(size_t x) {
+  return (global char*)x;
+}
+
+//SZ32: define{{.*}} i8 addrspace(3)* @test_add_local(i8 addrspace(3)* %x, i32 %y)
+//SZ32: getelementptr inbounds i8, i8 addrspace(3)* %{{.*}}, i32
+//SZ64: define{{.*}} i8 addrspace(3)* @test_add_local(i8 addrspace(3)* %x, i64 %y)
+//AMDONLY: trunc i64 %{{.*}} to i32
+//AMDONLY: getelementptr inbounds i8, i8 addrspace(3)* %{{.*}}, i32
+//SZ64ONLY: getelementptr inbounds i8, i8 addrspace(3)* %{{.*}}, i64
+local char* test_add_local(local char* x, ptrdiff_t y) {
+  return x + y;
+}
+
+//SZ32: define{{.*}} i8 addrspace(1)* @test_add_global(i8 addrspace(1)* %x, i32 %y)
+//SZ32: getelementptr inbounds i8, i8 addrspace(1)* %{{.*}}, i32
+//SZ64: define{{.*}} i8 addrspace(1)* @test_add_global(i8 addrspace(1)* %x, i64 %y)
+//SZ64: getelementptr inbounds i8, i8 addrspace(1)* %{{.*}}, i64
+global char* test_add_global(global char* x, ptrdiff_t y) {
+  return x + y;
+}
+
+//SZ32: define{{.*}} i32 @test_sub_local(i8 addrspace(3)* %x, i8 addrspace(3)* %y)
+//SZ32: ptrtoint i8 addrspace(3)* %{{.*}} to i32
+//SZ32: ptrtoint i8 addrspace(3)* %{{.*}} to i32
+//SZ64: define{{.*}} i64 @test_sub_local(i8 addrspace(3)* %x, i8 addrspace(3)* %y)
+//SZ64: ptrtoint i8 addrspace(3)* %{{.*}} to i64
+//SZ64: ptrtoint i8 addrspace(3)* %{{.*}} to i64
+ptrdiff_t test_sub_local(local char* x, local char *y) {
+  return x - y;
+}
+
+//SZ32: define{{.*}} i32 @test_sub_private(i8* %x, i8* %y)
+//SZ32: ptrtoint i8* %{{.*}} to i32
+//SZ32: ptrtoint i8* %{{.*}} to i32
+//SZ64: define{{.*}} i64 @test_sub_private(i8* %x, i8* %y)
+//SZ64: ptrtoint i8* %{{.*}} to i64
+//SZ64: ptrtoint i8* %{{.*}} to i64
+ptrdiff_t test_sub_private(private char* x, private char *y) {
+  return x - y;
+}
+
+//SZ32: define{{.*}} i32 @test_sub_mix(i8* %x, i8 addrspace(4)* %y)
+//SZ32: ptrtoint i8* %{{.*}} to i32
+//SZ32: ptrtoint i8 addrspace(4)* %{{.*}} to i32
+//SZ64: define{{.*}} i64 @test_sub_mix(i8* %x, i8 addrspace(4)* %y)
+//SZ64: ptrtoint i8* %{{.*}} to i64
+//SZ64: ptrtoint i8 addrspace(4)* %{{.*}} to i64
+ptrdiff_t test_sub_mix(private char* x, generic char *y) {
+  return x - y;
+}
+