[OpenCL] Pipe builtin functions

[pxli168]

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.

[pekka.jaaskelainen]

In D15914#322917, @pxli168 wrote:

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]

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]

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!

[pekka.jaaskelainen]

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]

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.

[pekka.jaaskelainen]

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]

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]

Add negative tests to hint all paths in the Semacheck.

[Anastasia]

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?

768

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]

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]

include/clang/Basic/Builtins.h

39

I just usede OCLC from OpenCL C.

[pxli168]

Rewrite some comment.

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

[keryell]

include/clang/Basic/Builtins.h

39

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

[Anastasia]

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

38

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

Ex:

invalid argument type to function write_pipe (expecting: 'int *')

[pxli168]

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

[pekka.jaaskelainen]

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]

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.

[pxli168]

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]

Add prefix "__" to CodeGen builtin function name.

[Anastasia]

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]

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]

Forget about get_pipe_num/max_packets builtin.

[Anastasia]

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

754

remove "therefore"

768

builtin def -> builtin def file

[pxli168]

Fix some small comments problems.

[pxli168]

lib/CodeGen/CGBuiltin.cpp

1974

Good idea! This can fit different target.

[Anastasia]

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?

[pxli168]

Remove some debug lines.

[pxli168]

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]

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.

[Anastasia]

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.jaaskelainen]

@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]

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

Thanks
Xiuli

[pxli168]

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

Thanks
Xiuli

[vpykhtin]

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.