- added restrictions to memory scope, order and volatile parameters
- 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)
- builtins renamed as requested
Details
Diff Detail
- Repository
- rC Clang
Event Timeline
lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
10094 | Can the pointer argument address space be checked here? |
lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
10094 | added code to check pointer and flaot parameters |
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.
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.
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.
Can the pointer argument address space be checked here?