This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] fixes for lds f32 builtins
ClosedPublic

Authored by dfukalov on Feb 14 2018, 3:14 AM.

Details

Summary
  1. added restrictions to memory scope, order and volatile parameters
  2. added custom processing for these builtins - currently is not used code, needed to switch off GCCBuiltin link to the builtins (ongoing change to llvm tree)
  3. builtins renamed as requested

Diff Detail

Repository
rC Clang

Event Timeline

dfukalov created this revision.Feb 14 2018, 3:14 AM
b-sumner added inline comments.Feb 14 2018, 7:32 AM
lib/CodeGen/CGBuiltin.cpp
10094

Can the pointer argument address space be checked here?

dfukalov updated this revision to Diff 134503.Feb 15 2018, 1:56 PM
dfukalov edited the summary of this revision. (Show Details)

diff updated as requested by reviewer

dfukalov marked an inline comment as done.Feb 15 2018, 1:59 PM
dfukalov added inline comments.
lib/CodeGen/CGBuiltin.cpp
10094

added code to check pointer and flaot parameters
though this part is not active at the moment since the builtins are not processed as custom.
going to switch to custom path by removing attribute of the intrinsics in llvm trunk in next step

dfukalov marked an inline comment as done.Feb 24 2018, 3:04 PM

ping...

Can’t you just change the description to be the LangAS value? I also thought these happened to be the same already

nhaehnle removed a subscriber: nhaehnle.Feb 25 2018, 7:37 AM

Can’t you just change the description to be the LangAS value? I also thought these happened to be the same already

Am I right that you mean to change the semantic of the addrspace number in a description string for all targets?

At the moment it's finally checked in ASTContext::getAddrSpaceQualType that LangAS is equal addrspace returned by QualType.
And for addrspace "2" specified in description, this QualType is "attribute((address_space(3))) float". And returns "11" since it's target addrspace, defined in LangAS after FirstTargetAddressSpace.
So ASTContext::getAddrSpaceQualType goes through check CanT.getAddressSpace() == AddressSpace and then hits assertions that the parameter already has addrspace specified.

I meant we could just change the value to whatever it happens to be for AMDGPU. That this is the language address space is surprising to me though, so maybe that should change

The problem is that if set addrspace "2" in description string, CanT.getAddressSpace() returns target addrspace value "11" (shifted in the enum) and compares it with input LangAS addrspace ("2", "opencl_local" in our case).
So I cannot set a number a description string that will be equal to LangAS addrspace "opencl_local".

Moreover, this change is preparation to move to custom processing of these builtins. Then I'm going to remove link (GCCBuiltin in IntrinsicsAMDGPU.td) from the llvm intrinsics definitions. Then I'll be able to switch on custom processing in cfe.

dfukalov updated this revision to Diff 136752.Mar 2 2018, 7:44 AM
dfukalov edited the summary of this revision. (Show Details)
dfukalov set the repository for this revision to rC Clang.

addrspace specifications are kept in descriptions strings

arsenm added a comment.Mar 9 2018, 1:58 PM

The problem is that if set addrspace "2" in description string, CanT.getAddressSpace() returns target addrspace value "11" (shifted in the enum) and compares it with input LangAS addrspace ("2", "opencl_local" in our case).
So I cannot set a number a description string that will be equal to LangAS addrspace "opencl_local".

Moreover, this change is preparation to move to custom processing of these builtins. Then I'm going to remove link (GCCBuiltin in IntrinsicsAMDGPU.td) from the llvm intrinsics definitions. Then I'll be able to switch on custom processing in cfe.

My real question was what happens if you put 11 in the description string?

My real question was what happens if you put 11 in the description string?

in this case CanT.getAddressSpace() returns target addrspace value "20" (also shifted in the enum by 9==LangAS::FirstTargetAddressSpace)

So again ASTContext::getAddrSpaceQualType decieds that these addrspaces are not equal and then asserts
a
The main reason of these change is to allow usage of the builtins with __local described pointers. Such pointers have LangAS AddressSpace in the ASTContext::getAddrSpaceQualType() equal to "2".
But we have no way to obtain this value from CanT.getAddressSpace() since it returns target addrspace and it is >= 10.

So I'm going to switch to custom processing of these builtins and the patch is preparation to be able to remove their links in llvm repo.

dfukalov updated this revision to Diff 142412.Apr 13 2018, 8:08 AM
dfukalov edited the summary of this revision. (Show Details)
yaxunl accepted this revision.May 14 2018, 8:14 AM

LGTM. Thanks!

This revision is now accepted and ready to land.May 14 2018, 8:14 AM

I'm looking at how the address space mapping works for builtins, and I think what's there is just uselessly broken and needs to be fixed. It seems to be operating under the assumption that the address spaces the target defines are totally disjoint from the language address spaces

lib/CodeGen/CGBuiltin.cpp
10100–10104

Condition backwards

I think the intent of the current code is for the address space to correspond to a "target address space" as if the user code used attribute((address_space(n))) to specify a pointer value. This is confusingly named, and different from the target address space selected for a LangAS. I think we need to add some mechanism for specifying the builtin is a LangAS ID. Since Ideally this would also work for multiple languages (e.g. cuda_constant or opencl_constant for the same builtin) I think there needs to be some callback triggered for the address space value. This possibly needs to be distinct from the current pointer descriptor to avoid breaking the possibility of user defined address spaces. There aren't really any other users of builtins with address spaces. NVPTX has some, but the tests seem to not actually try to use the declared address space and pass generic pointers to them.

This revision was automatically updated to reflect the committed changes.