Page MenuHomePhabricator

[OpenCL] Add to_{global|local|private} builtin functions.
ClosedPublic

Authored by yaxunl on May 4 2016, 1:07 PM.

Details

Summary

OpenCL builtin functions to_{global|local|private} accepts argument of pointer type to arbitrary pointee type, and return a pointer to the same pointee type in different addr space, i.e.

global gentype *to_global(gentype *p);

It is not desirable to declare it as

global void *to_global(void *);

in opencl header file since it misses diagnostics.

This patch implements these builtin functions as Clang builtin functions. In the builtin def file they are defined to have signature void*(void*). When handling call expressions, their declarations are re-written to have correct parameter type and return type corresponding to the call argument.

In codegen call to addr void *to_addr(void*) is generated with addrcasts or bitcasts to facilitate implementation in builtin library.

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl updated this revision to Diff 56192.May 4 2016, 1:07 PM
yaxunl retitled this revision from to [OpenCL] Add to_{global|local|private} builtin functions..
yaxunl updated this object.
yaxunl added reviewers: Anastasia, pxli168.
yaxunl added subscribers: cfe-commits, tstellarAMD.
yaxunl updated this revision to Diff 56194.May 4 2016, 1:12 PM

Update the test.

pxli168 edited edge metadata.May 4 2016, 10:11 PM

Could we output a generic function in CodeGen?
This seems to have no big difference to have a lot of declaration in an opencl c header file.

yaxunl added a comment.May 5 2016, 7:53 AM

Could we output a generic function in CodeGen?
This seems to have no big difference to have a lot of declaration in an opencl c header file.

What return type and argument type to use would you suggest for this generic function? If it is something like

i8 addrspace(1)* to_global(i8 addrspace(4)*)

then bitcasts will be needed, which is what we want to avoid.

These functions accept pointers to arbitrary user types, so they cannot be declared in a header file.

Anastasia edited edge metadata.May 6 2016, 1:22 PM

Could we output a generic function in CodeGen?
This seems to have no big difference to have a lot of declaration in an opencl c header file.

What return type and argument type to use would you suggest for this generic function? If it is something like

i8 addrspace(1)* to_global(i8 addrspace(4)*)

then bitcasts will be needed, which is what we want to avoid.

These functions accept pointers to arbitrary user types, so they cannot be declared in a header file.

I agree with Xiuli, if we don't have generic implementation with i8*, the libraries would have to contain every possible implementation of each AS conversion builtin including user defined types (which we don't even know beforehand).

I think that bitcast is not an issue in general as soon as passed type is casted from and back to the original type. This way we will disallow loosing the type information if let's say we compile:

int *ptr = ...
to_global(ptr) -> the return type is global int* (and not global void*)

Because we can create a bitcast to the original type.

include/clang/Basic/Builtins.def
1292 ↗(On Diff #56194)

I think we should not enable those builtins for all OpenCL versions as it prevents using those identifiers. I don't think they are reserved in the earlier OpenCL versions?

But I have a patch fixing it for all CL2.0 builtins. I can upload later. No need to fix now!

lib/CodeGen/CGBuiltin.cpp
2127 ↗(On Diff #56194)

Could you add "-" before "Address ..." to match general style.

2138 ↗(On Diff #56194)

I don't think mangling would work here at all. See my general comment to the generated prototype discussion.

lib/Sema/SemaChecking.cpp
480 ↗(On Diff #56194)

Should we also check that argument and return pointer type match?

Also we should probably check that the return type is in the right AS.

lib/Sema/SemaExpr.cpp
5222 ↗(On Diff #56194)

Is there any reason not to do normal IR CodeGen in CGBuiltin.cpp like we did for pipes:
http://reviews.llvm.org/D15914

Otherwise, it seems like we add an extra overhead for all the builtins and it doesn't seem any simpler code either.

yaxunl marked 5 inline comments as done.May 6 2016, 2:33 PM

I agree with Xiuli, if we don't have generic implementation with i8*, the libraries would have to contain every possible implementation of each AS conversion builtin including user defined types (which we don't even know beforehand).

I think that bitcast is not an issue in general as soon as passed type is casted from and back to the original type. This way we will disallow loosing the type information if let's say we compile:

int *ptr = ...
to_global(ptr) -> the return type is global int* (and not global void*)

Because we can create a bitcast to the original type.

These builtin functions can be lowered by a module pass which tracing the argument to determine the real addr space. In that case it does not matter that it can take infinite number of names.

However, there may be platforms on which these builtin functions are implemented in runtime library. I agree code generated as such will cause difficulty for them. Therefore I will emit call to i8 addrspace(1)* to_global (i8* addrspace(4)) and insert bitcasts. Also the name will not be mangled.

lib/Sema/SemaChecking.cpp
480 ↗(On Diff #56194)

The return type is generated based on the function and argument type. It always matches the argument type and has right AS. If the context of this call expr expects a different type, diagnostics will be emitted by the enclosing context.

lib/Sema/SemaExpr.cpp
5222 ↗(On Diff #56194)

In that case the type of the call expr does not need to be dynamically generated.

In this case, the type of the call expr depends on the argument type. If we don't dynamically generate the prototype of the builtin function, we have to insert bitcast, which results in an AST which is no longer a call expr and the diagnostics will become ugly.

The pointer type seems to be not that important, we can just cast it to any type we want.

test/SemaOpenCL/to_addr_builtin.cl
24 ↗(On Diff #56194)

should this return a NULL or a build error?

35 ↗(On Diff #56194)

I think this will be handled by pointer bitcast if we used some void* as well, the address space is the important thing of these built-ins.

yaxunl marked 4 inline comments as done.May 9 2016, 10:47 AM
yaxunl added inline comments.
test/SemaOpenCL/to_addr_builtin.cl
24 ↗(On Diff #56194)

the function is declared as

global gentype* to_addr(generic gentype*);

since constant pointer cannot be implicitly casted to a generic pointer (OpenCL v2.0 s6.5.5), this should cause a compilation error.

yaxunl updated this revision to Diff 56605.May 9 2016, 12:04 PM
yaxunl edited edge metadata.
yaxunl marked an inline comment as done.

Generate calls to void addr* to_addr(void*) with bitcasts.

You can update the SUMMARY of this diff as we are now using generic codegen output.
And there are some inline comments about spec.

test/SemaOpenCL/to_addr_builtin.cl
25 ↗(On Diff #56605)

But in Spec OpenCL V2.0 s6.13.9 the description for to_global is:

Returns a pointer that points to a region in the global address space if to_global can cast ptr to the global address space. Otherwise it returns NULL.

yaxunl updated this object.May 10 2016, 7:38 AM
yaxunl marked an inline comment as done.

Updated summary.

test/SemaOpenCL/to_addr_builtin.cl
25 ↗(On Diff #56605)

I think this is only for valid call expression. For constant pointer, it is an invalid call expression due to s6.5.5, so an error of invalid argument should be emitted.

If we allow passing constant pointer to this function, we violates s6.5.5.

Anastasia added inline comments.May 10 2016, 11:17 AM
lib/Sema/SemaChecking.cpp
480 ↗(On Diff #56605)

I just think you should simply set the function return type in line 487 based on the passed argument type (RT variable declared in line 477).

The situation I would like us to avoid here is:

int *gptr = ..;
local char *lptr = to_local(gptr);

So basically by calling to_local here, we allow conversion of not only an address space but also an underlaying type silently.

Could we add a test to make sure it's not happening.

Similarly, in C such situations trigger a warning:

int *ptr1 = ...;
char *ptr2 = ptr1; //expected-warning{{incompatible pointer types initializing 'char *' with an expression of type 'int *'}}
lib/Sema/SemaExpr.cpp
5222–5232 ↗(On Diff #56605)

I still don't understand the issue here.

In my understanding what we need is:
(1) Check the passed argument is a pointer and set return type to match the pointer type exactly in SemaChecking.
(2) Generate to_addr with i8* in CGBuiltin casting to and back the original type used in the call.

See my comment to lib/Sema/SemaChecking.cpp. I think if you set the return type there correctly we won't need this change here.

test/SemaOpenCL/to_addr_builtin.cl
21 ↗(On Diff #56605)

May be we could add here something like: expecting a generic pointer type...

pxli168 added inline comments.May 10 2016, 7:02 PM
test/SemaOpenCL/to_addr_builtin.cl
25 ↗(On Diff #56605)

A pointer that points to the constant address space cannot be cast or implicitly converted to the generic address space.

So if we can not cast, we maybe should return NULL?
@Anastasia What do you think about this?

yaxunl marked 8 inline comments as done.May 11 2016, 8:02 AM
yaxunl added inline comments.
lib/Sema/SemaChecking.cpp
480 ↗(On Diff #56605)

I added the above test. The current implementation is able to diagnose it.

Your suggested approach also works, so I will remove dynamic generation of to_addr declarations since it seems to be an overkill.

test/SemaOpenCL/to_addr_builtin.cl
25 ↗(On Diff #56605)

What the function returns is implemented by builtin library and Clang does not care about that. Clang only does syntax checking, type checking, etc. Since this is still a function, it is subject to all these rules. Unless we want to exempt this function from the type-checking rules. However I don't think the spec implies that.

yaxunl updated this revision to Diff 56915.May 11 2016, 8:06 AM
yaxunl updated this object.
yaxunl marked an inline comment as done.

Removed dynamic generation of builtin decl. Added a test for mismatched returned pointee types.

Anastasia added inline comments.May 11 2016, 11:49 AM
test/SemaOpenCL/to_addr_builtin.cl
26 ↗(On Diff #56915)

@Xiuli, I think Sam is right. Passing constant to generic violates AS conversion rules from s6.5.5. So error would be correct behavior of the compiler.

pxli168 accepted this revision.May 12 2016, 2:05 AM
pxli168 edited edge metadata.

LGTM

test/SemaOpenCL/to_addr_builtin.cl
26 ↗(On Diff #56915)

Yes, I think I just misunderstand some address space problem.

This revision is now accepted and ready to land.May 12 2016, 2:05 AM
Anastasia added inline comments.May 12 2016, 9:44 AM
include/clang/Basic/DiagnosticSemaKinds.td
501 ↗(On Diff #56915)

Could we move this to all the other OpenCL diagnostics to have them grouped together?

lib/Sema/SemaChecking.cpp
463 ↗(On Diff #56915)

Could we rename this please to SemaOpenCLBuiltinToAddr.

I think Pipe functions should be better renamed too at some point.

478 ↗(On Diff #56915)

Could the second check be written a bit simpler? I have a feeling something like this might work too:

RT->getPointeeType().getAddressSpace()
485 ↗(On Diff #56915)

Is canonical type really necessary here?

yaxunl updated this revision to Diff 57478.May 17 2016, 7:49 AM
yaxunl edited edge metadata.
yaxunl marked 8 inline comments as done.

Revised as Anastasia suggested.

Anastasia accepted this revision.May 20 2016, 12:51 PM
Anastasia edited edge metadata.

LGTM! Thanks!

This revision was automatically updated to reflect the committed changes.

Why couldn't this be declared in CLC header? There are multiple instances of declaring functions for all available types in libclc (see for example convert functions in convert.h).

The returned pointer should point to the same pointee type as the argument. Header file cannot guarantee that.

Sam

The returned pointer should point to the same pointee type as the argument. Header file cannot guarantee that.

Sam

how come? is there a possibility to have two different types using the same name?

The returned pointer should point to the same pointee type as the argument. Header file cannot guarantee that.

Sam

how come? is there a possibility to have two different types using the same name?

Because the pointee type is arbitrary, you can only define it as

global void* to_global(void*);

in the header file. Then you could have

int *a;
global double *b = to_global(a);

without diagnostics, but the spec requires that to_global(a) should have global int* type, therefore there should be some diagnostics.

The returned pointer should point to the same pointee type as the argument. Header file cannot guarantee that.

Sam

how come? is there a possibility to have two different types using the same name?

Because the pointee type is arbitrary, you can only define it as

global void* to_global(void*);

in the header file. Then you could have

int *a;
global double *b = to_global(a);

without diagnostics, but the spec requires that to_global(a) should have global int* type, therefore there should be some diagnostics.

this is not true. as I pointed out earlier, take a look at libclc headers. a lot functions are defined for multiple types while maintaining type safety.
there is no problem having TYPE * to_global(TYPE *), for every permissible CLC type, declared in headers without any builtin.

this is not true. as I pointed out earlier, take a look at libclc headers. a lot functions are defined for multiple types while maintaining type safety.
there is no problem having TYPE * to_global(TYPE *), for every permissible CLC type, declared in headers without any builtin.

This function allows user defined types. How do you declare that in a header file?

this is not true. as I pointed out earlier, take a look at libclc headers. a lot functions are defined for multiple types while maintaining type safety.
there is no problem having TYPE * to_global(TYPE *), for every permissible CLC type, declared in headers without any builtin.

This function allows user defined types. How do you declare that in a header file?

ah, now it makes sense, thanks. the void* argument let me astray.
I'd say you can still do it using typeof, but I see how builtin would be preferable to that.