Page MenuHomePhabricator

[OpenCL] Implement as_type operator as alias of __builtin_astype.
ClosedPublic

Authored by echuraev on Dec 28 2016, 5:49 AM.

Diff Detail

Event Timeline

echuraev updated this revision to Diff 82591.Dec 28 2016, 5:49 AM
echuraev retitled this revision from to [OpenCL] Implement as_type operator as alias of __builtin_astype..
echuraev updated this object.
echuraev added a reviewer: Anastasia.
echuraev added subscribers: bader, yaxunl, cfe-commits.
Anastasia edited edge metadata.Jan 3 2017, 12:17 PM

This has been discussed during the initial review for the header:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160425/157187.html

The main issue is after preprocessing the header the original function name is no longer available in diagnostics reported. The spec defines as_type as a builtin function and not a macro. Additionally your patch would allow as_type to be used with extra type (not only those defined in spec). Also I don't see the problem to implement as_type with just simply calling a builtin. It should be inlined later anyways.

echuraev abandoned this revision.Jan 12 2017, 10:06 PM
echuraev reclaimed this revision.Mar 10 2017, 6:17 AM
echuraev updated this revision to Diff 91327.

This has been discussed during the initial review for the header:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160425/157187.html

The main issue is after preprocessing the header the original function name is no longer available in diagnostics reported. The spec defines as_type as a builtin function and not a macro. Additionally your patch would allow as_type to be used with extra type (not only those defined in spec). Also I don't see the problem to implement as_type with just simply calling a builtin. It should be inlined later anyways.

I think that this patch is really necessary because in some cases previous implementation doesn't give a diagnostic. Please see test case that I have added to this review. With current implementation char variable will be cast to int. You can see the following part of llvm ir:

%0 = load i8, i8* %src, align 1
%conv = sext i8 %0 to i32
%call = call i32 @_Z6as_inti(i32 %conv) #2

So there is a bug and we didn't get error that the size of char isn't equal to size of int. Program is compiled without any problems.
If as_type functions will be defined in macro then we will have the following message:

error: invalid reinterpretation: sizes of 'float' and 'char' must match
int dst = as_int( src );
          ^~~~~~~~~~~~~~~
././llvm/tools/clang/lib/Headers/opencl-c-common.h:6615:21: note: expanded from macro 'as_int'
#define as_int(x) __builtin_astype((x), int)
                  ^                ~~~

This has been discussed during the initial review for the header:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160425/157187.html

The main issue is after preprocessing the header the original function name is no longer available in diagnostics reported. The spec defines as_type as a builtin function and not a macro. Additionally your patch would allow as_type to be used with extra type (not only those defined in spec). Also I don't see the problem to implement as_type with just simply calling a builtin. It should be inlined later anyways.

I think that this patch is really necessary because in some cases previous implementation doesn't give a diagnostic. Please see test case that I have added to this review. With current implementation char variable will be cast to int. You can see the following part of llvm ir:

%0 = load i8, i8* %src, align 1
%conv = sext i8 %0 to i32
%call = call i32 @_Z6as_inti(i32 %conv) #2

So there is a bug and we didn't get error that the size of char isn't equal to size of int. Program is compiled without any problems.
If as_type functions will be defined in macro then we will have the following message:

error: invalid reinterpretation: sizes of 'float' and 'char' must match
int dst = as_int( src );
          ^~~~~~~~~~~~~~~
././llvm/tools/clang/lib/Headers/opencl-c-common.h:6615:21: note: expanded from macro 'as_int'
#define as_int(x) __builtin_astype((x), int)
                  ^                ~~~

Why do you think this is a bug? It seems to follow standard behavior in C to promote char to int if required. Just like if you would have a C code:

int as_int(int i);
void foo() {
    char src = 1;
    int dst = as_int(src);
}

This code would complie and the same exactly IR would be generated.

bader added a comment.Mar 10 2017, 8:56 AM

Why do you think this is a bug? It seems to follow standard behavior in C to promote char to int if required. Just like if you would have a C code:

int as_int(int i);
void foo() {
    char src = 1;
    int dst = as_int(src);
}

This code would complie and the same exactly IR would be generated.

as_type is defined to be used for bit re-interpretation. (see 6.2.4.2 Reinterpreting Types Using as_type() and as_typen()). In this sense, it's exactly matches __bultin_astype built-in function.

Here are a few relevant OpenCL C language specification quotes from 6.2.4 section:

All data types described in tables 6.1 and 6.2 (except bool, half and void) may be also reinterpreted as another data type of the same size using the as_type() operator for scalar data types and the as_typen() operator for vector data types.

The usual type promotion for function arguments shall not be performed.

It is an error to use as_type() or as_typen() operator to reinterpret data to a type of a different number of bytes.

So, aliasing as_type to __builtin_astype provides these checks, whereas we can't do it for overloadable as_type function declarations.

I also would like to address your original concerns:

The main issue is after preprocessing the header the original function name is no longer available in diagnostics reported.

Actually diagnostics is able to provide a hint to exact code location in the header file where as_<type> is defined as macro, so user gets correct name of the operator in diagnostics as far as I know.

The spec defines as_type as a builtin function and not a macro.

To be precise, spec defines as_type as an operator. So, the best way to implement it would be to add a language support of such operator, but AFAIK aliasing to __bulitin_astype via macro is sufficient enough for OpenCL use cases.

Additionally your patch would allow as_type to be used with extra type (not only those defined in spec).

Not sure I get this. We defined limited set of as_* functions - only for types from tables 6.1 and 6.2 as specified by specification, so if OpenCL developer will try to call as_<type1>(type2), which is not defined in the header, compiler will report en error about calling undeclared function.

Also I don't see the problem to implement as_type with just simply calling a builtin. It should be inlined later anyways.

Yes, but this solution will not give us error checking as pre-processor solution.

Does it make sense?

Why do you think this is a bug? It seems to follow standard behavior in C to promote char to int if required. Just like if you would have a C code:

int as_int(int i);
void foo() {
    char src = 1;
    int dst = as_int(src);
}

This code would complie and the same exactly IR would be generated.

as_type is defined to be used for bit re-interpretation. (see 6.2.4.2 Reinterpreting Types Using as_type() and as_typen()). In this sense, it's exactly matches __bultin_astype built-in function.

Here are a few relevant OpenCL C language specification quotes from 6.2.4 section:

All data types described in tables 6.1 and 6.2 (except bool, half and void) may be also reinterpreted as another data type of the same size using the as_type() operator for scalar data types and the as_typen() operator for vector data types.

The usual type promotion for function arguments shall not be performed.

It is an error to use as_type() or as_typen() operator to reinterpret data to a type of a different number of bytes.

So, aliasing as_type to __builtin_astype provides these checks, whereas we can't do it for overloadable as_type function declarations.

I also would like to address your original concerns:

The main issue is after preprocessing the header the original function name is no longer available in diagnostics reported.

Actually diagnostics is able to provide a hint to exact code location in the header file where as_<type> is defined as macro, so user gets correct name of the operator in diagnostics as far as I know.

The spec defines as_type as a builtin function and not a macro.

To be precise, spec defines as_type as an operator. So, the best way to implement it would be to add a language support of such operator, but AFAIK aliasing to __bulitin_astype via macro is sufficient enough for OpenCL use cases.

Additionally your patch would allow as_type to be used with extra type (not only those defined in spec).

Not sure I get this. We defined limited set of as_* functions - only for types from tables 6.1 and 6.2 as specified by specification, so if OpenCL developer will try to call as_<type1>(type2), which is not defined in the header, compiler will report en error about calling undeclared function.

Also I don't see the problem to implement as_type with just simply calling a builtin. It should be inlined later anyways.

Yes, but this solution will not give us error checking as pre-processor solution.

Does it make sense?

From all the above arguments, I feel like the right approach would be to implement it as Clang builtin which closely matches the operator semantic in my opinion. We could of course reuse the implementation of __bultin_astype to avoid unnecessary extra work and code duplication.

Using the macro seems to me more like a workaround solution and overloaded functions don't seem to be entirely the right thing either. What do you think about it?

bader added a comment.Mar 15 2017, 3:35 AM

From all the above arguments, I feel like the right approach would be to implement it as Clang builtin which closely matches the operator semantic in my opinion. We could of course reuse the implementation of __bultin_astype to avoid unnecessary extra work and code duplication.

Using the macro seems to me more like a workaround solution and overloaded functions don't seem to be entirely the right thing either. What do you think about it?

I don't think we need another Clang built-in. __builtin_astype was added specifically for OpenCL needs (see rev. 132612).
Do you know better way to map astype operators (there are a lot of them as_<type>#) to single Clang built-in?

From all the above arguments, I feel like the right approach would be to implement it as Clang builtin which closely matches the operator semantic in my opinion. We could of course reuse the implementation of __bultin_astype to avoid unnecessary extra work and code duplication.

Using the macro seems to me more like a workaround solution and overloaded functions don't seem to be entirely the right thing either. What do you think about it?

I don't think we need another Clang built-in. __builtin_astype was added specifically for OpenCL needs (see rev. 132612).
Do you know better way to map astype operators (there are a lot of them as_<type>#) to single Clang built-in?

Unfortunately, __builtin_astype doesn't match the astype syntax from the spec exactly, which I wish it would. I don't feel strongly neither with macro (which messes up proper error reporting) nor with function declaration (for which implicit conversion would apply just like to other C functions). Having Clang builtin would feel the most natural way to me but I agree the way it is defined with the multiple distinct names for each type I am not going to advocate for it strongly.

lib/Headers/opencl-c.h
6588

Why do we have some of the overloads omitted? Would this cause any extra conversions? uchar -> char in this case?

bader added inline comments.Mar 15 2017, 9:26 AM
lib/Headers/opencl-c.h
6588

It's specific to how builtin_astype works. It drops first argument type and only cares about matching first argument size with the data type size provided as a second argument. So basically with single line we get all possible and impossible overloads of as_char(*).
This define will pass any variable type char,uchar to builtin_astype.

Anastasia accepted this revision.Mar 16 2017, 3:44 AM

LGTM! Thanks!

lib/Headers/opencl-c.h
6588

Ok, I see. I hope we won't get the "impossible" ones though. :)

This revision is now accepted and ready to land.Mar 16 2017, 3:44 AM
echuraev closed this revision.Mar 16 2017, 5:27 AM