Details
Diff Detail
Event Timeline
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.
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?
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 | ||
|---|---|---|
| 6582 | Why do we have some of the overloads omitted? Would this cause any extra conversions? uchar -> char in this case? | |
| lib/Headers/opencl-c.h | ||
|---|---|---|
| 6582 | 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(*). | |
LGTM! Thanks!
| lib/Headers/opencl-c.h | ||
|---|---|---|
| 6582 | Ok, I see. I hope we won't get the "impossible" ones though. :) | |
Why do we have some of the overloads omitted? Would this cause any extra conversions? uchar -> char in this case?