Page MenuHomePhabricator

[OpenCL] Pipe type support
ClosedPublic

Authored by pxli168 on Dec 16 2015, 11:40 PM.

Details

Summary

Support for OpenCL 2.0 pipe type.
This is a bug-fix version for bader's patch reviews.llvm.org/D14441

Diff Detail

Event Timeline

pxli168 retitled this revision from to [OpenCL] Pipe type support.
pxli168 updated this object.
pxli168 added a reviewer: pekka.jaaskelainen.
pxli168 added subscribers: cfe-commits, Anastasia, bader.
bader added inline comments.Dec 17 2015, 3:29 AM
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.
Particularly for this line Anastasia asked "Any reason for an empty comment line here?". I think it's not necessary.

pxli168 added inline comments.Dec 17 2015, 3:42 AM
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.

Anastasia added inline comments.Dec 24 2015, 8:33 AM
lib/AST/ASTContext.cpp
3147

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
OCL -> OpenCL

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?

pxli168 added inline comments.Dec 24 2015, 6:49 PM
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.
For example, the original read_pipe with 2 parameters is in the form

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.
Another way is to get the pipe element type form the -cl-kernel-arg-info, the pipe is as the type qual and the element type for the type.
So which one do you think is better? Add a size or using kernel-arg-info, I am now wondering if the built-in for pipe is really needed. Hope you can give me some advice.

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.

This comment was removed by pxli168.

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.

pxli168 added inline comments.Dec 28 2015, 6:44 AM
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?

pxli168 added inline comments.Dec 28 2015, 7:16 AM
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.
My plan is to use a patch to cover all these missing Sema check for OpenCL. That would be a hard work with the OpenCL Specification and would take a lot of time and may delay this patch, so could you accept this patch first and let me to finish the missing Sema check later to make sure cover more missing part.

pekka.jaaskelainen edited edge metadata.
This revision is now accepted and ready to land.Dec 28 2015, 7:34 AM
Anastasia added inline comments.Jan 4 2016, 11:00 AM
lib/AST/ASTContext.cpp
3147

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.

pxli168 added inline comments.Jan 4 2016, 7:24 PM
lib/AST/ASTContext.cpp
3147

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"
It seems this should be call “cannot combine with previous 'int' declaration specifier”
And the "error: missing actual type specifier for pipe" should be placed somewhere else.

pxli168 edited edge metadata.

Fix some unused diagnostic and wrong diagnostic. Add test for these diagnostics.

Anastasia accepted this revision.Jan 5 2016, 3:28 AM
Anastasia added a reviewer: Anastasia.

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.

pxli168 added inline comments.Jan 5 2016, 3:52 AM
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.

Anastasia added inline comments.Jan 5 2016, 9:19 AM
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.

pxli168 closed this revision.Jan 9 2016, 4:57 AM

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?

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();

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.

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 %s

It 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!