This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] AMDGCN: Fix size_t type
ClosedPublic

Authored by yaxunl on Aug 10 2016, 11:05 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl updated this revision to Diff 67554.Aug 10 2016, 11:05 AM
yaxunl retitled this revision from to [OpenCL] AMDGCN: Fix size_t type.
yaxunl updated this object.
yaxunl added reviewers: nhaustov, Anastasia.
yaxunl added subscribers: cfe-commits, tstellarAMD.
arsenm added a subscriber: arsenm.Aug 10 2016, 2:21 PM
arsenm added inline comments.
lib/Basic/Targets.cpp
2008–2010 ↗(On Diff #67554)

Should use ternary operator and check the triple arch instead

yaxunl updated this revision to Diff 67687.Aug 11 2016, 8:01 AM

Revised by Matt's comments.

Anastasia edited edge metadata.Aug 11 2016, 10:25 AM

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 marked an inline comment as done.Aug 11 2016, 11:29 AM

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.
  1. 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 added a subscriber: joey.Aug 12 2016, 7:55 AM
joey added inline comments.
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 added inline comments.Aug 16 2016, 12:17 PM
lib/CodeGen/CodeGenModule.cpp
101 ↗(On Diff #67687)

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

yaxunl updated this revision to Diff 68241.Aug 16 2016, 12:43 PM
yaxunl edited edge metadata.

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

arsenm added inline comments.Aug 16 2016, 1:49 PM
lib/CodeGen/CodeGenModule.h
664 ↗(On Diff #68241)

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

yaxunl updated this revision to Diff 68360.Aug 17 2016, 8:25 AM

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

Anastasia added inline comments.Aug 18 2016, 8:27 AM
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 marked an inline comment as done.Aug 18 2016, 8:35 AM
yaxunl added inline comments.
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 accepted this revision.Aug 18 2016, 8:44 AM
Anastasia edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Aug 18 2016, 8:44 AM
This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.