Support for OpenCL 2.0 pipe type.
This is a bug-fix version for bader's patch reviews.llvm.org/D14441
Details
Diff Detail
Event Timeline
include/clang/AST/Type.h | ||
---|---|---|
5020 | Xuili, could you apply the comments left by Pekka and Anastasia for the previous version of the patch (http://reviews.llvm.org/D14441). Most of the comments are valid for this patch also. |
include/clang/AST/Type.h | ||
---|---|---|
5020 | I have made some changes to their comments, but I found something still left here. I will fix them. |
lib/AST/ASTContext.cpp | ||
---|---|---|
3122 | I don't think this comment is much related here - a copy-paste issue perhaps? But I see this is replicated elsewhere. As mentioned in comment of http://reviews.llvm.org/D14441, I think we might want to look at refactoring the code that handles wrapper types in Clang. However, this shouldn't prevent this patch from being committed. | |
lib/CodeGen/CGOpenCLRuntime.cpp | ||
108 | Should there be any indication of pipe element type or at least its size? I.e should we differentiate between 'pipe char' or 'pipe int' in IR? Perhaps, it doesn't have to be a part of this commit, but it would still be useful to understand. | |
lib/CodeGen/CodeGenFunction.cpp | ||
565 | acess -> access | |
lib/Sema/TreeTransform.h | ||
5352 | Not clear why the namespace is being added here? i. e. seems to be old code. | |
test/CodeGenOpenCL/pipe_types.cl | ||
16 | Do we need to check reserve_id_t in every test? I feel like having it once would be enough. | |
test/SemaOpenCL/pipes-1.2-negative.cl | ||
2 | Could we add tests for checking CL2.0 pipes diagnostics added in this patch? |
lib/CodeGen/CGOpenCLRuntime.cpp | ||
---|---|---|
108 | There is one for the element size in the pipe related built-ins. But the built-in part refined the some of the built-in functions. int read_pipe(pipe gentype p, gentype *e); To get the indication for the pipe element type with in functions, we change it into this form int read_pipe(pipe gentype p, char *e,int sizeofgentype); This actually change the form of the OpenCL built-in functions, but can save a lot of things for the backend. | |
lib/CodeGen/CodeGenFunction.cpp | ||
565 | I will fix them, thank you. | |
lib/Sema/TreeTransform.h | ||
5352 | It seems to have no use, I will try to remove it and see if it can compile. | |
test/CodeGenOpenCL/pipe_types.cl | ||
16 | Good idea, I will remove some. | |
test/SemaOpenCL/pipes-1.2-negative.cl | ||
2 | Ok, I will add those checking for pipe access qualifier and element type. |
Fix some typo and remove unnecessary namespace, and add a new test case.
The Diff 43621 was on a wrong base, ignore it.
lib/CodeGen/CGOpenCLRuntime.cpp | ||
---|---|---|
108 | I'm not sure if touching the built-in fingerprints for this is a good idea. It might lead to problems and confusion. Cannot one pass pipes as arguments to user functions too? Are the fingerprints of those functions then modified accordingly? It might become messy. Because the packet size is known at host side, the pipes can be implemented as structs which holds the packet size as one of the member variables. The problem with this approach is how to exploit wider reads/writes instead of a scalar read/write loop + unpack/pack in case of vector datatypes. If the size is known only at runtime, one cannot easily generate vector reads/writes even if the element is a vector datatype and it would be efficient to keep the packet in a vector register as long as possible. For helping this I'd add a metadata which can be utilized at compile time to make reading/writing from the pipe faster. But in a way that is already an optimization, not a requirement, to make pipes work. The reading itself is platform dependent as the pipe can be even a hardware FIFO accessed using special instructions. |
lib/CodeGen/CGOpenCLRuntime.cpp | ||
---|---|---|
108 | This is what I'm worry about, I don't think we need to give much information about an opaque type in OpenCL. Actually we can get the element type from the metadata, and I think we can leave the optimization to the backend and let platform to choose which way to use for read/write pipe. And I think the built-in function support for the pipe in OpenCL-C is not necessary in the clang, what do you think? Though it can do some check in Sema check, they can also be done in some llvm pass in backend. If the built-in is really needed, I will send another patch based on this included built-in support for pipe. Thank you. |
lib/CodeGen/CGOpenCLRuntime.cpp | ||
---|---|---|
108 | As far as I've understood, no there's no need to add built-in function awareness to the frontend for this case. Sema checking/diagnostics is needed to ensure pipes are used only as function arguments, not local variables, for example. Is this patch missing it? |
lib/CodeGen/CGOpenCLRuntime.cpp | ||
---|---|---|
108 | I think there are a lot of Sema checking about OpenCL2.0 are missing, the pipe could not be a local variable is just one, there are also some check are missing for reserve_id_t in my tests these days. |
lib/AST/ASTContext.cpp | ||
---|---|---|
3122 | Could we update the comment here or otherwise I think it's best to be removed! | |
lib/CodeGen/CGOpenCLRuntime.cpp | ||
108 | I am Ok with not adding everything in one commit. We can revisit/modify things later on. | |
test/SemaOpenCL/invalid-pipes-cl2.0.cl | ||
4 | I can see more than one diagnostic being added in this patch, but the test covers only one. |
lib/AST/ASTContext.cpp | ||
---|---|---|
3122 | OK, just removed the copy paste one | |
lib/CodeGen/CGOpenCLRuntime.cpp | ||
108 | Do you think we need the make the read/write_pipe as a builtin in clang? Since these function need the pipe element type to be the same with the pointer pointee type. I find it hard to handle this in a backend. | |
test/SemaOpenCL/invalid-pipes-cl2.0.cl | ||
4 | It seems the auto related diagnostic is not necessary, since auto can not be used in OpenCL. And the "err_missing_actual_pipe_type" seems is used in a wrong place, it could not handle something like pipe p This will case "warning: type specifier missing, defaults to 'int'" But something like int pipe p will has "error: missing actual type specifier for pipe" |
LGTM!
lib/CodeGen/CGOpenCLRuntime.cpp | ||
---|---|---|
108 | Yes, I think we might, otherwise I don't see how we are going to allows any element type (including user defined types) for pipe without adding a template like functionality to C parsing. |
lib/CodeGen/CGOpenCLRuntime.cpp | ||
---|---|---|
108 | OK, I will send a patch only for these two builtin function (I don't think other pipe functions need to be built-in), and I think we can discuss about the function mangle for them, now I have a version without mangle and a hard code version. |
lib/CodeGen/CGOpenCLRuntime.cpp | ||
---|---|---|
108 | I am afraid we might need to add all of the pipe functions as builtins in order to allow them to be called with any pipe element type including user defined types. Otherwise, feel free to propose other ideas. If you have a patch already, let's start the review and we will see how to move forward. |
Now that our build bot is up and running again, I noticed that the newly added test pipe_types.cl fails on SystemZ:
/home3/uweigand/llvm/llvm-head/tools/clang/test/CodeGenOpenCL/pipe_types.cl:26:11: error: expected string not found in input // CHECK: define void @test5(%opencl.pipe_t* %p) ^ <stdin>:34:40: note: scanning from here define void @test4(%opencl.pipe_t* %p) #0 { ^ <stdin>:42:1: note: possible intended match here define void @test5(%opencl.pipe_t**) #0 { ^
For some reason, test5 gets an extra indirection in the input argument:
define void @test5(%opencl.pipe_t**) #0 { entry: %p.addr = alloca %opencl.pipe_t*, align 16 %p = load %opencl.pipe_t*, %opencl.pipe_t** %0, align 16 store %opencl.pipe_t* %p, %opencl.pipe_t** %p.addr, align 16 ret void }
Are there supposed to be platform-specific differences in OpenCL types?
No, this shouldn't happen! There shouldn't be any difference in emitted IR depending on a platform. I also don't see any code in CodeGen that could specialize.
Is there a way to reproduce your compilation? Passing any triple?
I see the problem when using:
clang --target=s390x-linux -S -O0 -emit-llvm -std=CL2.0 -o - pipe_types.cl
Using --target=i386-linux instead results in the expected output.
Yes, I see that it happens only for some vector types. The CodeGen follows quite different paths for both targets. And in the case of s390x-linux-gnu it ends up in ABIArgInfo::Indirect case of the second switch statement where an additional pointer will be added here:
ArgTypes[FirstIRArg] = LTy->getPointerTo();
I don't have enough knowledge at the moment of this ABI to tell whether it's Ok, but the generated code appears to be completely wrong.
As a temporary workaround to fix your build bot, I suggest you to add '-triple x86_64-linux-gnu' into RUN line of the test:
-// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s +// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - -triple x86_64-linux-gnu %s | FileCheck %s
It has already been done before in OpenCL tests as we don't support most of targets anyways.
In the meantime, I will try to see if there could be a better fix for this.
Btw, just to be clear in my previous comment I was referring to the 2nd switch statement in CodeGenTypes::GetFunctionType that contains the line:
ArgTypes[FirstIRArg] = LTy->getPointerTo();
Well, yes, on SystemZ we pass 16-byte vector types via implicit reference (using ABIArgInfo::Indirect) in the default case, i.e. when targeting an older CPU without SIMD. But the code generated for OpenCL pipes doesn't actually seem to correctly implement that convention either ... (It does work for normal calls via C/C++/...)
As a temporary workaround to fix your build bot, I suggest you to add '-triple x86_64-linux-gnu' into RUN line of the test:
-// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s +// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - -triple x86_64-linux-gnu %s | FileCheck %sIt has already been done before in OpenCL tests as we don't support most of targets anyways.
OK, I've checked this in as rev. 259183. Thanks!
Xuili, could you apply the comments left by Pekka and Anastasia for the previous version of the patch (http://reviews.llvm.org/D14441). Most of the comments are valid for this patch also.
Particularly for this line Anastasia asked "Any reason for an empty comment line here?". I think it's not necessary.