This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Pipe builtin functions
ClosedPublic

Authored by pxli168 on Jan 5 2016, 10:05 PM.

Details

Summary

Support for the pipe built-in functions for OpenCL 2.0.
The pipe builtin functions may have infinite kinds of element types, one approach
would be to just generate calls that would always use generic types such as void*.
This patch is based on bader's opencl support patch on SPIR-V branch.

Diff Detail

Event Timeline

pxli168 updated this revision to Diff 44086.Jan 5 2016, 10:05 PM
pxli168 retitled this revision from to [OpenCL] Pipe builtin functions.
pxli168 updated this object.
pxli168 added subscribers: cfe-commits, bader.
Anastasia added inline comments.Jan 7 2016, 7:21 AM
include/clang/Basic/Builtins.def
1255

Could we put reference to spec section?

1256

Is variadic right here? Should be generic perhaps?

1257

I think it would make sense to have this as LANGBUILTIN as it's only available in OpenCL.

1260

From reserve_read_pipe onwards the builtins have a fixed number of arguments, and don't need to be variadic.

include/clang/Basic/DiagnosticSemaKinds.td
7643

add OpenCL v2.0

lib/CodeGen/CGBuiltin.cpp
2028

Closing ) is missing.

2033

So why do we need to mangle at all since the generated function signature will always have the same parameter types?

2081

So why do we need to mangle at all since the generated function signatures will always have the same parameter types?

lib/CodeGen/CGOpenCLRuntime.h
55 ↗(On Diff #44086)

I am not very convinced on this solution of implementing a separate mangler. Also how would it play together with existing mangling schemes?

Additionally how would definitions of builtin with user defined types appear in the BIF libraries? I am not clear at the moment.

One approach would be to just generate calls that would always use generic types: opaque type for pipe and void* for a packet and avoid the mangling completely. That's what I think is done already (see previous comment), just not clear why mangling is added too.

lib/Sema/SemaChecking.cpp
267

Not sure why we have an image access here?

268

What happens if access qualifier is not provided?

According to spec it should default to read_only!

274

Should we also be checking that read_write is not used with pipe?

282

Should be a generic name rather than image!

291

Remove this commented code

296

Should you be checking find() instead of startwith()?

297

Could move this in the above if statement to avoid rechecking.

307

To adhere the coding standard:
call -> Call, idx -> Idx

Please, check other places too.

309

Argidx -> ArgIdx

316

Aren't first and second checking the same thing?

378

Do we need setArg as well? Not sure though...

445

No call->setType() here?

766

What two functions do you refer to?

I don't think we have var args here i .e the number of arguments is fixed to 2 and not a variable number!

I think the checking is due to generic argument types to be passed.

So here is the summary about the inline comments:

  1. What kind of prototype should codegen output for these functions?
  2. What function names should these builtins have in llvm ir?
  3. Access for pipe use image's for now, and will be refined.
  4. What rules should we follow to check the access for the group functions, since specification does not tell?
include/clang/Basic/Builtins.def
1255

OK, I will add a reference.

1256

That sounds better.

1257

Good idea.

1260

I think the pipe and reserve_id_t does not have a macro here, and that is why there is a full semacheck for all arguments.

lib/CodeGen/CGBuiltin.cpp
2033

That's one thing I want to discuss about, do you think it is good to have the builtin functions have the same parameter?
Actually I have another version without mangle and the last size, and the other thing is that if we need to pass the packet size here if we know that the packet type is the same with the pointer form the semacheck. I think the packet size can be got from the pipe as we can get it with metadata of pipe.

Hope we can make some consensus about this that can make backend easier!

lib/CodeGen/CGOpenCLRuntime.h
55 ↗(On Diff #44086)

Here is some code for the generic types you can see, it is just hard coded the function mangle for the void*.
Also this is part of the experiment code from bader, as you asked that we can discuss about it in patch, I just update them to see if this hard code can be accepted.

The builtin codegen part of this patch is only a draft, I actually have no idea about how to output these builtin,
if you think the opaque type and generic type could works, could you give some advice about the function name?
Should we follow the spir specification?

lib/Sema/SemaChecking.cpp
267

That was a fixme in last commit about the read_only write_only and read_write. Since pipe use the access that used to be image only, I think we'd better still use it and refine it to something like OpenCLTypeAcces after this patch to make it more clearer in commit.

268

Yes, this was handled later when get a null.
We can refine it to a better form later.

274

We only return valid for the read_only and write_only, and return error for others, maybe that should be handled with the access after refined version.

282

Yes, will be done in another commit to make it clearer.

291

That was something confused me, if we check startwith, we only check the read_pipe function, but reserve/commit_read/write_pipe also need to be a read_only/write_only.
And the specification does not write about if the pipe to be read_only or write_only for the work/sub_group functions.

What is your idea about this?
Shall we ignore the access for the group functions?

292

Here we check for null access.

297

Ok, good idea.

307

Thank you for your remind, i just copy the name from some function above, maybe they are out of date. I will change all in this patch.

316

It seems so, I will delete one.
Thank you.

378

Maybe not, it is declared in the builtin def I think.

445

I is set in switch and case.

766

I think this was for the two function above, I will fix this comment.

pxli168 updated this revision to Diff 44312.Jan 7 2016, 11:59 PM
  1. Refine the def of BUILTIN with LANGBUITIN with new OCLC_LANG
  2. Remove mangler for the hard coded builtin function name
  3. Fix some bugs.
Anastasia edited edge metadata.Jan 8 2016, 7:47 AM

Do you have any tests? We won't be able to commit without. Also having them would help understanding what this modification does.

include/clang/Basic/Builtins.def
1260

That's right! I am just saying that we don't have to make it variadic, so you can change "v." -> "v" and "i." -> "i"

lib/CodeGen/CGBuiltin.cpp
2033

I am still not convinced we need to mangle here at all. It seems we mainly have only one prototype for those functions apart from read/write_pipe in which case we can generate two versions of that function:

read_pipe_2(...) - for two parameters case
read_pipe_4(...) - for four parameters case

Having a mangler would complicate the implementation. And if used it would be better to adhere to conventional scheme (i.e. calling getMangledName()). But that won't work here as we are generating parameters different from originally specified. I don't see any need or benefit in mangling at the moment since the function signatures are all known here.

Regarding the packet size, I prefer to pass it in. This way we can allow simple BIF implementation without using any metadata magic. Usage of metadata often implies adding non-conventional compilation mechanisms somewhere that are best to avoid.

lib/Sema/SemaChecking.cpp
268

Let's rename this new function in this commit and I agree with renaming an attribute later on. May be leaving TODO explaining that would be good.

282

Ah yes, I see. It should be fine!

I wonder could we squeeze this in before the next week's LLVM 3.8 branching? It would be great.

lib/CodeGen/CGBuiltin.cpp
2033

We just need to remember that passing the packet size as an argument means touching all the user functions having pipes as arguments too.

I'm still in the understanding that the compile time packet size info is needed only for optimization purposes, and thus could be a metadata which your implementation can just ignore for starters, or convert to a parameter before code emission? Touching the initial function finger prints for optimization reasons sounds a bit too intrusive. I'd rather leave the decision what to do with it to consumer of the Clang's output.

lib/CodeGen/CGOpenCLRuntime.h
55 ↗(On Diff #44312)

Additionally how would definitions of builtin with user defined types appear in the BIF libraries?

This is a good question. read_pipe should just work for any type of any size, thus we cannot just generate a new function for all possible sizes in advance, thus what Anastasia suggests here makes sense to me:

One approach would be to just generate calls that would always use generic types

If now there was an additional parameter (always a constant) that stores the type's size it would not help much as one would need to generate a big switch...case that optimizes the access based on the packet size in case of a software pipe or a compiler pass that looks into that argument and generates (a call to) an optimized version?

I think combining the Anastasia proposed generic read/write_pipe with the metadata (that points to the packet's inner type or its size?) would be the best solution (so far).

pxli168 added inline comments.Jan 9 2016, 1:17 AM
include/clang/Basic/Builtins.def
1260

If that means there is no arguments, and may be misleading.

lib/CodeGen/CGBuiltin.cpp
2033

That could be fine I think, the function name do not need mangle if we use generic type.

lib/Sema/SemaChecking.cpp
268

OK, it will make the function more easy to understand. And I will add a FIXME or TODO here to explain the misleading imageaccess.

And how do you think about the access check for the work_group functions?
The specification does not say clearly, but it seems they should also obey the same rule with read/write_pipe.

Then maybe I should hurry up to see if I can finish the Semacheck and this patch in time.
By the way, I just got the access to the llvm svn, can I just commit the pipe type patch directly as I see you all accepted it. Or I should send it to the cfe-commit first?

lib/CodeGen/CGOpenCLRuntime.h
55 ↗(On Diff #44312)

Ok, I will send a patch in the best way so far.

By the way, I just got the access to the llvm svn, can I just commit the pipe type patch directly as I see you all accepted it. Or I should send it to the cfe-commit first?

I'm afraid we won't get more reviews from cfe-commit so IMO just commit it in case it was accepted.

pxli168 updated this revision to Diff 44404.Jan 9 2016, 5:13 AM
pxli168 updated this object.
pxli168 edited edge metadata.
  1. Add tests for builtin function CodeGen and Semacheck
  2. Add TODO for what we need to do about access qualifier
  3. Fix some bugs found in writing

Hope we cans squeeze these patch into llvm 3.8

Anastasia added inline comments.Jan 11 2016, 8:10 AM
include/clang/Basic/Builtins.def
1260

Ok, this way it means variable number of arg which isn't right either. But I see some other builtins do the same.

I think a better approach would be to add pipe as a modifier and a way to represent any gentype in builtins declarations here to be able to specify a signature properly. Although it seems it won't be used for anything but documentation purposes.

Also adding the overloaded variant for read/write_pipes explicitly might be a good idea (similar to sync_fetch_and_add).

But I see that not all builtins follow this approach, so it's up to you.

lib/CodeGen/CGBuiltin.cpp
2033

Why do we need to modify user defined functions? Only builtins will have this extra parameter.

I think packet size would be useful for builtin implementation to know the number of bytes to be read/written. I don't see how to implement it correctly otherwise. As mentioned earlier relying on the metadata is not a conventional compilation approach and should be avoided if possible.

lib/Sema/SemaChecking.cpp
268

Yes, they should. It seems however it has already been checked here?

274

Makes sense!

291

I agree the spec doesn't require that checking but I think it's just being imprecise in the description. If you are in doubt you can raise a bug with Khronos to clarify. This might result in additional latency.

I think it makes sense though to check all of them.

test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
5

I think we need to test all the builtins!

lib/CodeGen/CGBuiltin.cpp
2033

The pipe struct can have the packet size in its header before the actual ring buffer or whatever, which can be used as a fallback unless the compiler can optimize it to a larger access. Correct implementation thus doesn't require a "hidden parameter". Adding it as a compile time hidden constant argument should help the optimizers, that's of course true, but I don't think it's strictly required.

If you think having a special behavior for the built-ins calls isn't problematic, then fine, I'm not having so strong opinion on this.

Anastasia added inline comments.Jan 11 2016, 10:41 AM
lib/CodeGen/CGBuiltin.cpp
2033

So where would this size be initialized/set? Note that host code might have different type sizes.

I am thinking that having a size parameter makes the generated function more generic since we lose information about the type here and recovering it might involve extra steps. I am currently not clear about how to do that.

lib/CodeGen/CGBuiltin.cpp
2033

https://www.khronos.org/registry/cl/sdk/2.0/docs/man/xhtml/clCreatePipe.html sets the packet size in creation. We have a prototype implementation working using that. But that generic software version uses a byte loop which is painfully slow, which of course should be optimized to wide loads/stores whenever possible.

If we want to optimize it, it means inlining the builtin call and then deriving the size for the loop at the compile time after which it can be vectorized (assuming the packet fifo is aligned). Where this size constant comes from is the question under discussion -- you propose having it as an additional hidden parameter to the calls and I propose a metadata. Yours optimizes automatically but requires special casing (when calling a builtin, not user function, add this magic parameter), mine requires a new optimization that takes the MD in account and converts it to a constant after inlining the built-in.

I'm open for both as I do not really know how much trouble the special casing will bring and I appreciate that optimizations might just work, but in my understanding the size info is strictly not required, but very useful for optimization.

pxli168 added inline comments.Jan 11 2016, 6:42 PM
include/clang/Basic/Builtins.def
1260

I think this maybe OK for a def file, the parameter is strictly checked in Sema.

lib/CodeGen/CGBuiltin.cpp
2033

I am thinking that having a size parameter makes the generated function more generic since we lose information about the type here and recovering it might involve extra steps. I am currently not clear about how to do that.

I know what Anastasia is worried about, but actually I should say that you can simply create a pass for the pipe function that get this information for the metadata, you can use MDNode to get these information pretty easy. That is what I am doing now.

lib/Sema/SemaChecking.cpp
291

I will keep it here then, and we can change it once the Khronos have some feeback.

test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
5

Should we test the negative case for all of the builtin? There are for group of cases and I think maybe just 4 groups with read/write that touches all of the cases maybe enough.

pxli168 updated this revision to Diff 44601.Jan 11 2016, 8:57 PM

Add negative tests to hint all paths in the Semacheck.

Anastasia added inline comments.Jan 12 2016, 4:01 AM
include/clang/Basic/Builtins.def
1255

Could you remove one -?

include/clang/Basic/Builtins.h
39

May be: OCLC -> OCL

lib/CodeGen/CGBuiltin.cpp
2002

remove "(Verified by Sema)"

2033

Yes. I do believe that a pass would recover this, but on the other hand it is an extra step. Every such step can potentially add to compilation time which can easily be avoided by already generating code in the format that directly sets the right value.

"Yours optimizes automatically but requires special casing (when calling a builtin, not user function, add this magic parameter)"

I am not sure what is special casing. All pipe functions are Clang builtins now and will be generated to calls to internal functions (for those we can define the format the way we need it to be).

Anyways, I see what you mean this is an optimization and not needed for supporting correct functionality. I still think it's nicer not to force compiler developer to create extra passes for something we can provide in a suitable format from the beginning.

I don't want to block this change for that reason though. This is a small modification and so can be done later as well.

2039

remove "(Verified by Sema)"

lib/Sema/SemaChecking.cpp
267

Add space after TODO:

291

Why the TODO here?

771

Could you improve the comment? Not clear why do we need to override the type...

test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
6

We should tests all code added to the compiler.

34

remove : from error string

35

remove : from error string

36

remove : from error string

37

remove : from error string

pxli168 marked 4 inline comments as done.Jan 12 2016, 5:27 AM
pxli168 added inline comments.
include/clang/Basic/Builtins.def
1255

Removed.

lib/Sema/SemaChecking.cpp
291

TODO after get clarify from Khronos.

test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
37

what do you mean by

remove : from error string

pxli168 added inline comments.Jan 12 2016, 5:33 AM
include/clang/Basic/Builtins.h
39

I just usede OCLC from OpenCL C.

pxli168 updated this revision to Diff 44624.Jan 12 2016, 5:42 AM

Rewrite some comment.

Can this patch be in llvm3.8 release if it is commited after 13th Jan?

keryell added inline comments.
include/clang/Basic/Builtins.h
39

Yes, it may be useful to differentiate OpenCL C from the on-going OpenCL C++

Anastasia added inline comments.Jan 13 2016, 4:03 AM
test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
37

Sometimes you use ":" where it's not needed.

Ex:

invalid argument type to function write_pipe (expecting: 'int *')
pxli168 updated this revision to Diff 44826.Jan 13 2016, 7:21 PM

Fix format of diag err string, remove useless ':' after expecting.

OK. Seems the LLVM 3.8 branching was done, it might be hard to get this in, but should we try and ask the release manager?

lib/CodeGen/CGBuiltin.cpp
1981

Shouldn't these have underscores, because they are not reserved OpenCL keywords/builtins? Cannot user functions be called read_pipe_2 or write_pipe_2?

Anastasia added inline comments.Jan 14 2016, 3:42 AM
lib/CodeGen/CGBuiltin.cpp
1981

Yes, definitely! Add prefix "__" to each generated name! I think this should be done for all of them!

lib/Sema/SemaChecking.cpp
291

Please, remove this TODO. Have you created the bug to Khronos already?

It seems sensible enough the access qualifiers must match. How can reserve/commit_read_pipe be called with a write_only pipe?

378

I don't think we have any arguments there. It's all declared as variadic. But it seems to work without setting it. So I am not sure it's necessary to set it at all.

I think we'd better to ask about to add this in. The pipe type support actually have no use without builtins to read or write it.

lib/CodeGen/CGBuiltin.cpp
1981

I think that's a good idea!

lib/Sema/SemaChecking.cpp
291

OK, I will. I tried to create an account to do, but it seems I could not receive the email. I will attach the bug report web here once I created one.

378

The return type of int and uint is set in the builtin.def.

pxli168 updated this revision to Diff 44881.Jan 14 2016, 7:57 AM

Add prefix "__" to CodeGen builtin function name.

Anastasia added inline comments.Jan 14 2016, 10:53 AM
lib/CodeGen/CGBuiltin.cpp
2082

prepend "__" here too!

lib/Sema/SemaChecking.cpp
291

Ok, I think we can proceed with this review. You can let the spec be clarified on the side.

378

I am asking about setArg and not setType!

pxli168 marked 2 inline comments as done.Jan 14 2016, 8:25 PM
pxli168 added inline comments.
lib/Sema/SemaChecking.cpp
378

Oh, I just mistook your meaning. It seems we don't need that as we have to covert them in CGbuiltin.

pxli168 updated this revision to Diff 44965.Jan 14 2016, 9:27 PM

Forget about get_pipe_num/max_packets builtin.

A few nitpicks here, otherwise it seems we are in a good shape. I hope this will hit trunk soon. :)

lib/CodeGen/CGBuiltin.cpp
1966

Can you put a comment with the spec reference here.

1974

Please, avoid hard coded constants.

You can use getTargetAddressSpace() method of ASTContext to obtain a generic AS number, something like:

unsigned GenericAS = getContext().getTargetAddressSpace(LangAS::opencl_generic);
llvm::Type Int8Ty = ...(..., GenericAS);
1978

I don't think this comment is any relevant and can be removed!

1983

the type is not really variadic! May be better to say:

"Creating a generic function type to be able to call with any builtin or user defined type."

lib/Sema/SemaChecking.cpp
286

modifier -> qualifier

394

reverse -> reserve

434

Packetes -> Packets

757

remove "therefore"

771

builtin def -> builtin def file

pxli168 updated this revision to Diff 45234.Jan 19 2016, 1:44 AM
pxli168 updated this object.
pxli168 marked 9 inline comments as done.

Fix some small comments problems.

pxli168 added inline comments.Jan 19 2016, 1:45 AM
lib/CodeGen/CGBuiltin.cpp
1974

Good idea! This can fit different target.

Anastasia accepted this revision.Jan 19 2016, 2:55 AM
Anastasia edited edge metadata.

LGTM! Could you please remove the line with printf before committing (see comment above)?

lib/CodeGen/CGBuiltin.cpp
1976

Could you remove this line with printf please?

This revision is now accepted and ready to land.Jan 19 2016, 2:55 AM
pxli168 updated this revision to Diff 45258.Jan 19 2016, 7:25 AM
pxli168 edited edge metadata.

Remove some debug lines.

Hi pekka,
What is your opinion about this patch?
And how could we ask the release manager to squeeze this patch into LLVM release 3.8?
The pipe in OpneCL can only be handled by builtin functions, and this patch is needed for the pipe support.

Thanks
Xiuli

pekka.jaaskelainen edited edge metadata.

LGTM too. Good job. You can try and ask the release manager if you can sneak it in. It's rather container patch so there might be a chance, but I don't know the policy at this stage of the release.

Could you please copy us in CC?

@pekka, do you think it would make sense to include a description of the new OpenCL features in the release note too? Or shall we wait for the completion of OpenCL 2.0 support (hopefully in next release)? Could do both as well I guess. :)

@pekka, do you think it would make sense to include a description of the new OpenCL features in the release note too? Or shall we wait for the completion of OpenCL 2.0 support (hopefully in next release)? Could do both as well I guess. :)

Yes I think it would make sense. It's good to advertise that the OpenCL support is progressing.

pxli168 closed this revision.Jan 25 2016, 6:10 PM

Hi,
Email was sent, hope this can be merged into llvm release3.8.

Thanks
Xiuli

Hi Anastasia/Pekka,
https://www.khronos.org/bugzilla/show_bug.cgi?id=1459
Bug report has been sent.

Thanks
Xiuli

Hi,

is there an intention to make this SPIR 2.0 (provisional) conformant? This would require changing resulting builtin names, changing pipe argument to "struct opencl.pipe*" and adding two trailing "size, alignment" parameters. If there is an interest for this a can try to submit a patch for review next week.