Page MenuHomePhabricator

Try to make builtin address space declarations not useless
ClosedPublic

Authored by arsenm on May 21 2018, 12:05 PM.

Details

Summary

The way address space declarations for builtins currently work
is nearly useless. The code assumes the address spaces used for
builtins is a confusingly named "target address space" from user
code using attribute((address_space(N))) that matches
the builtin declaration. There's no way to use this to declare
a builtin that returns a language specific address space.
The terminology used is highly cofusing since it has nothing
to do with the the address space selected by the target to use
for a language address space.

This feature is essentially unused as-is. AMDGPU and NVPTX
are the only in-tree targets attempting to use this. The AMDGPU
builtins certainly do not behave as intended (i.e. all of the
builtins returning pointers can never compile because the numbered
address space never matches the expected named address space).

The NVPTX builtins are missing tests for some, and the others
seem to rely on an implicit addrspacecast.

Change the used address space for builtins based on a target
hook to allow using a language address space for a builtin.
This allows the same builtin declaration to be used for multiple
languages with similarly purposed address spaces (e.g. the same
AMDGPU builtin can be used in OpenCL and CUDA even though the
constant address spaces are arbitarily different).

This breaks the possibility of using arbitrary numbered
address spaces alongside the named address spaces for builtins.
If this is an issue we probably need to introduce another builtin
declaration character to distinguish language address spaces from
so-called "target address spaces".

Diff Detail

Event Timeline

arsenm created this revision.May 21 2018, 12:05 PM
jlebar added a reviewer: tra.May 21 2018, 1:39 PM
Anastasia added inline comments.May 22 2018, 10:26 AM
include/clang/Basic/BuiltinsAMDGPU.def
49

Do you plan to provide the support for it later? Or if else perhaps we should elaborate more what's to be done.

include/clang/Basic/TargetInfo.h
1170

Can you add a comment please to explain what the function is for?

lib/AST/ASTContext.cpp
9355–9359

Could we check against LangAS::Default instead of removing this completely.

lib/CodeGen/CGBuiltin.cpp
3708

Would this be correct for OpenCL? Should we use isAddressSpaceSupersetOf helper instead? Would it also sort the issue with constant AS (at least for OpenCL)?

test/CodeGenOpenCL/numbered-address-space.cl
37

__attribute__((address_space(N))) is not an OpenCL feature and I think it's not specified in C either? But I think generally non matching address spaces don't compile in Clang. So it might be useful to disallow this?

tra added a comment.May 22 2018, 4:20 PM

CUDA does not expose explicit AS on clang size. All pointers are treated as generic and we infer specific address space only in LLVM.
__nvvm_atom_*_[sg]_* builtins should probably be removed as they are indeed useless without pointers with explicit AS and NVCC itself does not have such builtins either. Instead, we should convert the generic AS builtin to address-space specific instruction somewhere in LLVM.

Using attribute((address_space()) should probably produce an error during CUDA compilation.

bjope added a subscriber: bjope.May 31 2018, 1:32 PM
arsenm added inline comments.Jun 6 2018, 12:17 PM
include/clang/Basic/BuiltinsAMDGPU.def
49

I'm not sure. I don't know how to best enforce this

lib/AST/ASTContext.cpp
9355–9359

I don't think that really make sense, since that would be leaving this the same. I don't really need it for this patch, but I fundamentally think specifying address space 0 is different from an unspecified address space. According to the description for builtins, if no address space is specified than any address space will be accepted. This is different from a builtin requiring address space 0

lib/CodeGen/CGBuiltin.cpp
3708

The issue I mentioned for the other builtin is that it modifies the memory, and doesn't have to do with the casting.

At this point the AddrSpaceCast has to be emitted. The checking if the cast is legal I guess would be in the SemaExpr part. I know at one point I was trying to use isAddressSpaceSupersetOf in rewriteBuiltinFunctionDecl, but there was some problem with that. I think it didn't make sense with the magic where the builtin without an address space is supposed to accept any address space or something along those lines.

test/CodeGenOpenCL/numbered-address-space.cl
37

I'm pretty sure it's a C extension. The way things seem to work now is address spaces are accepted anywhere and everywhere.

arsenm updated this revision to Diff 150179.Jun 6 2018, 12:26 PM

Rebase and add comment

Anastasia added inline comments.Jun 12 2018, 4:32 AM
include/clang/Basic/BuiltinsAMDGPU.def
49

The only way I guess if we list overloads with each address space explicitly (apart from constant)? Or may be with the use of generic AS, although that will only work for CL2.0.

lib/AST/ASTContext.cpp
9355–9359

I thought Default AS was meant to be for the case no AS is specified but I guess it doesn't work the same in Builtins specification syntax.

lib/CodeGen/CGBuiltin.cpp
3708

Yes, I think Sema has to check it before indeed. I am not sure it works right with OpenCL rules though for the Builtin functions. Would it make sense to add a negative test for this then?

arsenm added inline comments.Jun 26 2018, 12:19 PM
lib/CodeGen/CGBuiltin.cpp
3708

I'm not sure what this test would look like. Do you mean a test that erroneously is accepted now?

Anastasia added inline comments.Jul 5 2018, 8:50 AM
lib/CodeGen/CGBuiltin.cpp
3708

Ok, so at this point you are trying to change generation of bitcast to addrspacecast which makes sense to me. Do we still need a bitcast though?

I think addrspacecast can be used to convert type and address space too:

The ‘addrspacecast‘ instruction converts ptrval from pty in address space n to type pty2 in address space m.

It would be nice to add proper Sema checking for Builtins for address space of pointers in OpenCL mode, but this might be more work.

test/CodeGenOpenCL/numbered-address-space.cl
37

Yes, the line below should give an error for OpenCL?

generic int* generic_ptr = local_ptr;
arsenm updated this revision to Diff 154561.Jul 9 2018, 3:32 AM

Add sema test for numbered address spaces

arsenm added inline comments.Jul 9 2018, 3:34 AM
lib/CodeGen/CGBuiltin.cpp
3708

I think the canonical form is to use the bitcast for the type pointer conversion, and then separate the addrspacecast. I think instcombine splits these apart

test/CodeGenOpenCL/numbered-address-space.cl
37

This does not error. The wording of the spec seems to leave some interpretation for other address spaces. It whitelists the valid address spaces for implicit casts, and blacklists constant for implicit or explicit casts. My reading between the lines is that an explicit cast would be OK. I think this is a separate fix since this is independent from the builtins

Anastasia accepted this revision.Jul 27 2018, 8:56 AM

LGTM from OpenCL side! Thanks!

test/SemaOpenCL/numbered-address-space.cl
10

Ideally this is not governed by any specification. Generic compiler support for such addrspacecast is not possible but vendor can implement custom support. I think we can leave it as is until we have better idea how this should be supported.

This revision is now accepted and ready to land.Jul 27 2018, 8:56 AM
yaxunl requested changes to this revision.Jul 27 2018, 9:17 AM
yaxunl added inline comments.
lib/Basic/Targets/AMDGPU.h
398

I am wondering how this would work for CUDA/HIP.

Let's say a builtin is supposed to return a pointer to addrspace 4.

Now in HIP this builtin is returning a pointer to addrspace 0.

How would that work?

This revision now requires changes to proceed.Jul 27 2018, 9:17 AM
yaxunl added inline comments.Jul 27 2018, 9:43 AM
include/clang/Basic/TargetInfo.h
1178

I think this function is not needed.

Although CUDA/HIP uses address spaces in codegen, but it does not use named address spaces in sema and AST. Named address space is not in the AST types of CUDA/HIP, therefore there is no point of mapping target address space back to language address space for CUDA/HIP.

CUDA/HIP should be just like other address-space-agnostic language and always use getLangASFromTargetAS(AS).

yaxunl added inline comments.Jul 27 2018, 9:59 AM
test/CodeGenOpenCL/builtins-amdgcn.cl
3

Please remove this line since we no longer use opencl in triple.

In D47154#1108813, @tra wrote:

CUDA does not expose explicit AS on clang size. All pointers are treated as generic and we infer specific address space only in LLVM.
__nvvm_atom_*_[sg]_* builtins should probably be removed as they are indeed useless without pointers with explicit AS and NVCC itself does not have such builtins either. Instead, we should convert the generic AS builtin to address-space specific instruction somewhere in LLVM.

Using attribute((address_space()) should probably produce an error during CUDA compilation.

Sometimes we need to call functions defined in our device library which is written in OpenCL. Some function have pointer arguments in non-zero address space. To declare these functions in CUDA/HIP we need to use __attribute__((address_space())). We use C-style cast to cast pointers in CUDA/HIP to a non-zero address space and pass them to the functions. I think __attribute__((address_space())) is still needed for this situation.

arsenm added inline comments.Jul 29 2018, 2:13 AM
include/clang/Basic/TargetInfo.h
1178

This is necessary to insert the correct addrspacecast, otherwise the builtins CUDA test asserts. getLangASFromTargetAS returns the user specified addrspace

arsenm updated this revision to Diff 157885.Jul 29 2018, 2:15 AM

Remove old run line

yaxunl accepted this revision.Jul 30 2018, 4:36 AM

LGTM. Thanks.

I missed the addr space casts you added to CodeGenFunction::EmitBuiltinExpr. With those casts it should work.

For other downstream address space agnostic languages, e.g. (HCC), I guess they need to add similar hooks to use this feature.

This revision is now accepted and ready to land.Jul 30 2018, 4:36 AM
arsenm closed this revision.Aug 2 2018, 5:15 AM

r338707