[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.

[Anastasia]

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.

[pxli168]

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]

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]

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!

[pekka.jaaskelainen]

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]

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.

[pxli168]

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.

[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?

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]

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

37

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

757

remove "therefore"

771

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.

include/clang/Basic/Builtins.h

   CXX_LANG = 0x4,  // builtin for cplusplus only.
   OBJC_LANG = 0x8, // builtin for objective-c and objective-c++
   MS_LANG = 0x10,  // builtin requires MS mode.
+  OCLC_LANG = 0x20,// builtin for OpenCL C only.
   ALL_LANGUAGES = C_LANG | CXX_LANG | OBJC_LANG, // builtin for all languages.
   ALL_GNU_LANGUAGES = ALL_LANGUAGES | GNU_LANG,  // builtin requires GNU mode.
   ALL_MS_LANGUAGES = ALL_LANGUAGES | MS_LANG     // builtin requires MS mode.

include/clang/Basic/Builtins.def

 BUILTIN(__builtin_nontemporal_store, "v.", "t")
 BUILTIN(__builtin_nontemporal_load, "v.", "t")
 
+// OpenCL v2.0 s6.13.16, s9.17.3.5 - Pipe functions.
+// We need the generic prototype, since the packet type could be anything.
+LANGBUILTIN(read_pipe, "i.", "tn", OCLC_LANG)
+LANGBUILTIN(write_pipe, "i.", "tn", OCLC_LANG)
+
+LANGBUILTIN(reserve_read_pipe, "i.", "tn", OCLC_LANG)
+LANGBUILTIN(reserve_write_pipe, "i.", "tn", OCLC_LANG)
+
+LANGBUILTIN(commit_write_pipe, "v.", "tn", OCLC_LANG)
+LANGBUILTIN(commit_read_pipe, "v.", "tn", OCLC_LANG)
+
+LANGBUILTIN(sub_group_reserve_read_pipe, "i.", "tn", OCLC_LANG)
+LANGBUILTIN(sub_group_reserve_write_pipe, "i.", "tn", OCLC_LANG)
+
+LANGBUILTIN(sub_group_commit_read_pipe, "v.", "tn", OCLC_LANG)
+LANGBUILTIN(sub_group_commit_write_pipe, "v.", "tn", OCLC_LANG)
+
+LANGBUILTIN(work_group_reserve_read_pipe, "i.", "tn", OCLC_LANG)
+LANGBUILTIN(work_group_reserve_write_pipe, "i.", "tn", OCLC_LANG)
+
+LANGBUILTIN(work_group_commit_read_pipe, "v.", "tn", OCLC_LANG)
+LANGBUILTIN(work_group_commit_write_pipe, "v.", "tn", OCLC_LANG)
+
+LANGBUILTIN(get_pipe_num_packets, "Ui.", "tn", OCLC_LANG)
+LANGBUILTIN(get_pipe_max_packets, "Ui.", "tn", OCLC_LANG)
+
 #undef BUILTIN
 #undef LIBBUILTIN
 #undef LANGBUILTIN

include/clang/Basic/DiagnosticSemaKinds.td

 def err_opencl_implicit_vector_conversion : Error<
   "implicit conversions between vector types (%0 and %1) are not permitted">;
 
+// OpenCL v2.0 s6.13.6 -- Builtin Pipe Functions
+def err_opencl_builtin_pipe_first_arg : Error<
+  "first argument to %0 must be a pipe type">;
+def err_opencl_builtin_pipe_arg_num : Error<
+    "invalid number of arguments to function: %0">;
+def err_opencl_builtin_pipe_invalid_arg : Error<
+  "invalid argument type to function %0 (expecting: %1)">;
+def err_opencl_builtin_pipe_invalid_access_modifier : Error<
+  "invalid pipe access modifier (expecting %0)">;
+
 // OpenCL Section 6.8.g
 def err_opencl_unknown_type_specifier : Error<
   "OpenCL does not support the '%0' %select{type qualifier|storage class specifier}1">;

lib/Basic/Builtins.cpp

   bool MSModeUnsupported =
       !LangOpts.MicrosoftExt && (BuiltinInfo.Langs & MS_LANG);
   bool ObjCUnsupported = !LangOpts.ObjC1 && BuiltinInfo.Langs == OBJC_LANG;
-  return !BuiltinsUnsupported && !MathBuiltinsUnsupported &&
+  bool OclCUnsupported = !LangOpts.OpenCL && BuiltinInfo.Langs == OCLC_LANG;
+  return !BuiltinsUnsupported && !MathBuiltinsUnsupported && !OclCUnsupported &&
          !GnuModeUnsupported && !MSModeUnsupported && !ObjCUnsupported;
 }
 

lib/CodeGen/CGBuiltin.cpp

       return RValue::get(llvm::ConstantExpr::getBitCast(GV, CGM.Int8PtrTy));
     break;
   }
+
+  case Builtin::BIread_pipe:
+  case Builtin::BIwrite_pipe: {
+    llvm::LLVMContext &Ctxt = getLLVMContext();
+    Value *Arg0 = EmitScalarExpr(E->getArg(0)),
+          *Arg1 = EmitScalarExpr(E->getArg(1));
+
+    // Type of the generic packet parameter.
+    llvm::Type *I8PTy = llvm::PointerType::get(llvm::Type::getInt8Ty(Ctxt), 4U);
+
+    // Testing which overloaded version we should generate the call for.
+    if (2U == E->getNumArgs()) {
+      // The name is hard-coded for now, since we would like to avoid the
+      // AST rewrite for the infinite user define types.
+      const char *Name =
+          (BuiltinID == Builtin::BIread_pipe) ? "read_pipe_2" : "write_pipe_2";
+      // Re-Creating the function type for this call, since the original type
+      // is variadic, we make them to be generic type.
+      llvm::Type *ArgTys[] = {Arg0->getType(), I8PTy};
+      llvm::FunctionType *FTy = llvm::FunctionType::get(
+          Int32Ty, llvm::ArrayRef<llvm::Type *>(ArgTys), false);
+      Value *BCast = Builder.CreatePointerCast(Arg1, I8PTy);
+      return RValue::get(Builder.CreateCall(
+          CGM.CreateRuntimeFunction(FTy, Name), {Arg0, BCast}));
+    } else {
+      assert(4 == E->getNumArgs() &&
+             "Illegal number of parameters to pipe function");
+      const char *Name =
+          (BuiltinID == Builtin::BIread_pipe) ? "read_pipe_4" : "write_pipe_4";
+
+      llvm::Type *ArgTys[] = {Arg0->getType(), Arg1->getType(), Int32Ty, I8PTy};
+      Value *Arg2 = EmitScalarExpr(E->getArg(2)),
+            *Arg3 = EmitScalarExpr(E->getArg(3));
+      llvm::FunctionType *FTy = llvm::FunctionType::get(
+          Int32Ty, llvm::ArrayRef<llvm::Type *>(ArgTys), false);
+      Value *BCast = Builder.CreatePointerCast(Arg3, I8PTy);
+      // We know the third argument is an integer type, but we may need to cast
+      // it to i32.
+      if (Arg2->getType() != Int32Ty)
+        Arg2 = Builder.CreateZExtOrTrunc(Arg2, Int32Ty);
+      return RValue::get(Builder.CreateCall(
+          CGM.CreateRuntimeFunction(FTy, Name), {Arg0, Arg1, Arg2, BCast}));
+    }
+  }
+  case Builtin::BIreserve_read_pipe:
+  case Builtin::BIreserve_write_pipe:
+  case Builtin::BIwork_group_reserve_read_pipe:
+  case Builtin::BIwork_group_reserve_write_pipe:
+  case Builtin::BIsub_group_reserve_read_pipe:
+  case Builtin::BIsub_group_reserve_write_pipe: {
+    // Composing the mangled name for the function.
+    const char *Name;
+    if (BuiltinID == Builtin::BIreserve_read_pipe)
+      Name = "reserve_read_pipe";
+    else if (BuiltinID == Builtin::BIreserve_write_pipe)
+      Name = "reserve_write_pipe";
+    else if (BuiltinID == Builtin::BIwork_group_reserve_read_pipe)
+      Name = "work_group_reserve_read_pipe";
+    else if (BuiltinID == Builtin::BIwork_group_reserve_write_pipe)
+      Name = "work_group_reserve_write_pipe";
+    else if (BuiltinID == Builtin::BIsub_group_reserve_read_pipe)
+      Name = "sub_group_reserve_read_pipe";
+    else
+      Name = "sub_group_reserve_write_pipe";
+
+    Value *Arg0 = EmitScalarExpr(E->getArg(0)),
+          *Arg1 = EmitScalarExpr(E->getArg(1));
+    llvm::Type *ReservedIDTy = ConvertType(getContext().OCLReserveIDTy);
+
+    // Building the generic function prototype.
+    llvm::Type *ArgTys[] = {Arg0->getType(), Int32Ty};
+    llvm::FunctionType *FTy = llvm::FunctionType::get(
+        ReservedIDTy, llvm::ArrayRef<llvm::Type *>(ArgTys), false);
+    // We know the second argument is an integer type, but we may need to cast
+    // it to i32.
+    if (Arg1->getType() != Int32Ty)
+      Arg1 = Builder.CreateZExtOrTrunc(Arg1, Int32Ty);
+    return RValue::get(
+        Builder.CreateCall(CGM.CreateRuntimeFunction(FTy, Name), {Arg0, Arg1}));
+  }
+  case Builtin::BIcommit_read_pipe:
+  case Builtin::BIcommit_write_pipe:
+  case Builtin::BIwork_group_commit_read_pipe:
+  case Builtin::BIwork_group_commit_write_pipe:
+  case Builtin::BIsub_group_commit_read_pipe:
+  case Builtin::BIsub_group_commit_write_pipe: {
+    const char *Name;
+    if (BuiltinID == Builtin::BIcommit_read_pipe)
+      Name = "commit_read_pipe";
+    else if (BuiltinID == Builtin::BIcommit_write_pipe)
+      Name = "commit_write_pipe";
+    else if (BuiltinID == Builtin::BIwork_group_commit_read_pipe)
+      Name = "work_group_commit_read_pipe";
+    else if (BuiltinID == Builtin::BIwork_group_commit_write_pipe)
+      Name = "work_group_commit_write_pipe";
+    else if (BuiltinID == Builtin::BIsub_group_commit_read_pipe)
+      Name = "sub_group_commit_read_pipe";
+    else
+      Name = "sub_group_commit_write_pipe";
+
+    Value *Arg0 = EmitScalarExpr(E->getArg(0)),
+          *Arg1 = EmitScalarExpr(E->getArg(1));
+
+    // Building the generic function prototype.
+    llvm::Type *ArgTys[] = {Arg0->getType(), Arg1->getType()};
+    llvm::FunctionType *FTy =
+        llvm::FunctionType::get(llvm::Type::getVoidTy(getLLVMContext()),
+                                llvm::ArrayRef<llvm::Type *>(ArgTys), false);
+
+    return RValue::get(
+        Builder.CreateCall(CGM.CreateRuntimeFunction(FTy, Name), {Arg0, Arg1}));
+  }
+  case Builtin::BIget_pipe_num_packets:
+  case Builtin::BIget_pipe_max_packets: {
+    const char *Name;
+    if (BuiltinID == Builtin::BIget_pipe_num_packets)
+      Name = "get_pipe_num_packets";
+    else
+      Name = "get_pipe_max_packets";
+
+    // Building the generic function prototype.
+    Value *Arg0 = EmitScalarExpr(E->getArg(0));
+    llvm::Type *ArgTys[] = {Arg0->getType()};
+    llvm::FunctionType *FTy = llvm::FunctionType::get(
+        Int32Ty, llvm::ArrayRef<llvm::Type *>(ArgTys), false);
+
+    return RValue::get(
+        Builder.CreateCall(CGM.CreateRuntimeFunction(FTy, Name), {Arg0}));
+  }
   }
 
   // If this is an alias for a lib function (e.g. __builtin_sin), emit

lib/Sema/SemaChecking.cpp

   return false;
 }
 
+/// Returns readable name for a call.
+static StringRef getFunctionName(CallExpr *Call) {
+  return cast<FunctionDecl>(Call->getCalleeDecl())->getName();
+}
+
+/// Returns OpenCL access qual.
+// TODO: Refine OpenCLImageAccessAttr to OpenCLAccessAttr since pipe can use
+// it too
+static OpenCLImageAccessAttr *getOpenCLArgAccess(const Decl *D) {
+  if (D->hasAttr<OpenCLImageAccessAttr>())
+    return D->getAttr<OpenCLImageAccessAttr>();
+  return nullptr;
+}
+
+/// Returns true if pipe element type is different from the pointer.
+static bool checkOpenCLPipeArg(Sema &S, CallExpr *Call) {
+  const Expr *Arg0 = Call->getArg(0);
+  // First argument type should always be pipe.
+  if (!Arg0->getType()->isPipeType()) {
+    S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_first_arg)
+        << getFunctionName(Call) << Arg0->getSourceRange();
+    return true;
+  }
+  OpenCLImageAccessAttr *AccessQual =
+      getOpenCLArgAccess(cast<DeclRefExpr>(Arg0)->getDecl());
+  // Validates the access modifier is compatible with the call.
+  // From OpenCL C Specification 6.13.16 the access qualifiers for
+  // pipe should only be read_only and write_only, and assumed to
+  // be read_only if no qualifier is specified.
+  bool isValid = true;
+  // TODO: Now we check for all the pipe builtin with access qualifier, but in
+  // OpenCL spec does not tell about group functions, need to fix after get
+  // clarify from Khronos.
+  bool ReadOnly = getFunctionName(Call).find("read") != StringRef::npos;
+  if (ReadOnly)
+    isValid = AccessQual == nullptr || AccessQual->isReadOnly();
+  else
+    isValid = AccessQual != nullptr && AccessQual->isWriteOnly();
+  if (!isValid) {
+    const char *AM = ReadOnly ? "read_only" : "write_only";
+    S.Diag(Arg0->getLocStart(),
+           diag::err_opencl_builtin_pipe_invalid_access_modifier)
+        << AM << Arg0->getSourceRange();
+    return true;
+  }
+
+  return false;
+}
+
+/// Returns true if pipe element type is different from the pointer.
+static bool checkOpenCLPipePacketType(Sema &S, CallExpr *Call, unsigned Idx) {
+  const Expr *Arg0 = Call->getArg(0);
+  const Expr *ArgIdx = Call->getArg(Idx);
+  const PipeType *PipeTy = cast<PipeType>(Arg0->getType());
+  const Type *EltTy = PipeTy->getElementType().getTypePtr();
+  const PointerType *ArgTy =
+      dyn_cast<PointerType>(ArgIdx->getType().getTypePtr());
+  // The Idx argument should be a pointer and the type of the pointer and
+  // the type of pipe element should also be the same.
+  if (!ArgTy || EltTy != ArgTy->getPointeeType().getTypePtr()) {
+    S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_invalid_arg)
+        << getFunctionName(Call)
+        << S.Context.getPointerType(PipeTy->getElementType())
+        << ArgIdx->getSourceRange();
+    return true;
+  }
+  return false;
+}
+
+// \brief Performs semantic analysis for the read/write_pipe call.
+// \param S Reference to the semantic analyzer.
+// \param Call A pointer to the builtin call.
+// \return True if a semantic error has been found, false otherwise.
+static bool SemaBuiltinRWPipe(Sema &S, CallExpr *Call) {
+  // Two kinds of read/write pipe
+  // From OpenCL C Specification 6.13.16.2 the built-in read/write
+  // functions have following forms.
+  switch (Call->getNumArgs()) {
+  case 2: {
+    if (checkOpenCLPipeArg(S, Call))
+      return true;
+    // The call with 2 arguments should be
+    // read/write_pipe(pipe T, T*)
+    // check packet type T
+    if (checkOpenCLPipePacketType(S, Call, 1))
+      return true;
+  } break;
+
+  case 4: {
+    if (checkOpenCLPipeArg(S, Call))
+      return true;
+    // The call with 4 arguments should be
+    // read/write_pipe(pipe T, reserve_id_t, uint, T*)
+    // check reserve_id_t
+    if (!Call->getArg(1)->getType()->isReserveIDT()) {
+      S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_invalid_arg)
+          << getFunctionName(Call) << S.Context.OCLReserveIDTy
+          << Call->getArg(1)->getSourceRange();
+      return true;
+    }
+
+    // check the index
+    const Expr *Arg2 = Call->getArg(2);
+    if (!Arg2->getType()->isIntegerType() &&
+        !Arg2->getType()->isUnsignedIntegerType()) {
+      S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_invalid_arg)
+          << getFunctionName(Call) << S.Context.UnsignedIntTy
+          << Arg2->getSourceRange();
+      return true;
+    }
+
+    // check packet type T
+    if (checkOpenCLPipePacketType(S, Call, 3))
+      return true;
+  } break;
+  default:
+    S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_arg_num)
+        << getFunctionName(Call) << Call->getSourceRange();
+    return true;
+  }
+
+  return false;
+}
+
+// \brief Performs a semantic analysis on the {work_group_/sub_group_
+//        /_}reserve_{read/write}_pipe
+// \param S Reference to the semantic analyzer.
+// \param Call The call to the builtin function to be analyzed.
+// \return True if a semantic error was found, false otherwise.
+static bool SemaBuiltinReserveRWPipe(Sema &S, CallExpr *Call) {
+  if (checkArgCount(S, Call, 2))
+    return true;
+
+  if (checkOpenCLPipeArg(S, Call))
+    return true;
+
+  // check the reverse size
+  if (!Call->getArg(1)->getType()->isIntegerType() &&
+      !Call->getArg(1)->getType()->isUnsignedIntegerType()) {
+    S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_invalid_arg)
+        << getFunctionName(Call) << S.Context.UnsignedIntTy
+        << Call->getArg(1)->getSourceRange();
+    return true;
+  }
+
+  return false;
+}
+
+// \brief Performs a semantic analysis on {work_group_/sub_group_
+//        /_}commit_{read/write}_pipe
+// \param S Reference to the semantic analyzer.
+// \param Call The call to the builtin function to be analyzed.
+// \return True if a semantic error was found, false otherwise.
+static bool SemaBuiltinCommitRWPipe(Sema &S, CallExpr *Call) {
+  if (checkArgCount(S, Call, 2))
+    return true;
+
+  if (checkOpenCLPipeArg(S, Call))
+    return true;
+
+  // check reserve_id_t
+  if (!Call->getArg(1)->getType()->isReserveIDT()) {
+    S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_invalid_arg)
+        << getFunctionName(Call) << S.Context.OCLReserveIDTy
+        << Call->getArg(1)->getSourceRange();
+    return true;
+  }
+
+  return false;
+}
+
+// \brief Performs a semantic analysis on the call to built-in Pipe
+//        Query Functions.
+// \param S Reference to the semantic analyzer.
+// \param Call The call to the builtin function to be analyzed.
+// \return True if a semantic error was found, false otherwise.
+static bool SemaBuiltinPipePacketes(Sema &S, CallExpr *Call) {
+  if (checkArgCount(S, Call, 1))
+    return true;
+
+  if (!Call->getArg(0)->getType()->isPipeType()) {
+    S.Diag(Call->getLocStart(), diag::err_opencl_builtin_pipe_first_arg)
+        << getFunctionName(Call) << Call->getArg(0)->getSourceRange();
+    return true;
+  }
+
+  return false;
+}
+
 ExprResult
 Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
                                CallExpr *TheCall) {

 
     TheCall->setType(Context.VoidPtrTy);
     break;
+  case Builtin::BIread_pipe:
+  case Builtin::BIwrite_pipe:
+    // Since those two functions are declared with var args, therefore we need
+    // a semantic check for the argument.
+    if (SemaBuiltinRWPipe(*this, TheCall))
+      return ExprError();
+    break;
+  case Builtin::BIreserve_read_pipe:
+  case Builtin::BIreserve_write_pipe:
+  case Builtin::BIwork_group_reserve_read_pipe:
+  case Builtin::BIwork_group_reserve_write_pipe:
+  case Builtin::BIsub_group_reserve_read_pipe:
+  case Builtin::BIsub_group_reserve_write_pipe:
+    if (SemaBuiltinReserveRWPipe(*this, TheCall))
+      return ExprError();
+    // Since return type of reserve_read/write_pipe built-in function is
+    // reserve_id_t, which is not defined in the builtin def, we used int as
+    // return type and need to override the return type of these functions
+    TheCall->setType(Context.OCLReserveIDTy);
+    break;
+  case Builtin::BIcommit_read_pipe:
+  case Builtin::BIcommit_write_pipe:
+  case Builtin::BIwork_group_commit_read_pipe:
+  case Builtin::BIwork_group_commit_write_pipe:
+  case Builtin::BIsub_group_commit_read_pipe:
+  case Builtin::BIsub_group_commit_write_pipe:
+    if (SemaBuiltinCommitRWPipe(*this, TheCall))
+      return ExprError();
+    break;
+  case Builtin::BIget_pipe_num_packets:
+  case Builtin::BIget_pipe_max_packets:
+    if (SemaBuiltinPipePacketes(*this, TheCall))
+      return ExprError();
+    break;
 
   }
 

test/CodeGenOpenCL/pipe_builtin.cl

+// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+
+// CHECK: %opencl.pipe_t = type opaque
+// CHECK: %opencl.reserve_id_t = type opaque
+
+void test1(read_only pipe int p, global int *ptr) {
+  // CHECK: call i32 @read_pipe_2(%opencl.pipe_t* %{{.*}}, i8 addrspace(4)* %{{.*}})
+  read_pipe(p, ptr);
+  // CHECK: call %opencl.reserve_id_t* @reserve_read_pipe(%opencl.pipe_t* %{{.*}}, i32 {{.*}})
+  reserve_id_t rid = reserve_read_pipe(p, 2);
+  // CHECK: call i32 @read_pipe_4(%opencl.pipe_t* %{{.*}}, %opencl.reserve_id_t* %{{.*}}, i32 {{.*}}, i8 addrspace(4)* %{{.*}})
+  read_pipe(p, rid, 2, ptr);
+  // CHECK: call void @commit_read_pipe(%opencl.pipe_t* %{{.*}}, %opencl.reserve_id_t* %{{.*}})
+  commit_read_pipe(p, rid);
+}
+
+void test2(write_only pipe int p, global int *ptr) {
+  // CHECK: call i32 @write_pipe_2(%opencl.pipe_t* %{{.*}}, i8 addrspace(4)* %{{.*}})
+  write_pipe(p, ptr);
+  // CHECK: call %opencl.reserve_id_t* @reserve_write_pipe(%opencl.pipe_t* %{{.*}}, i32 {{.*}})
+  reserve_id_t rid = reserve_write_pipe(p, 2);
+  // CHECK: call i32 @write_pipe_4(%opencl.pipe_t* %{{.*}}, %opencl.reserve_id_t* %{{.*}}, i32 {{.*}}, i8 addrspace(4)* %{{.*}})
+  write_pipe(p, rid, 2, ptr);
+  // CHECK: call void @commit_write_pipe(%opencl.pipe_t* %{{.*}}, %opencl.reserve_id_t* %{{.*}})
+  commit_write_pipe(p, rid);
+}
+
+void test3(read_only pipe int p, global int *ptr) {
+  // CHECK: call %opencl.reserve_id_t* @work_group_reserve_read_pipe(%opencl.pipe_t* %{{.*}}, i32 {{.*}})
+  reserve_id_t rid = work_group_reserve_read_pipe(p, 2);
+  // CHECK: call void @work_group_commit_read_pipe(%opencl.pipe_t* %{{.*}}, %opencl.reserve_id_t* %{{.*}})
+  work_group_commit_read_pipe(p, rid);
+}
+
+void test4(write_only pipe int p, global int *ptr) {
+  // CHECK: call %opencl.reserve_id_t* @work_group_reserve_write_pipe(%opencl.pipe_t* %{{.*}}, i32 {{.*}})
+  reserve_id_t rid = work_group_reserve_write_pipe(p, 2);
+  // CHECK: call void @work_group_commit_write_pipe(%opencl.pipe_t* %{{.*}}, %opencl.reserve_id_t* %{{.*}})
+  work_group_commit_write_pipe(p, rid);
+}
+
+void test5(read_only pipe int p, global int *ptr) {
+  // CHECK: call %opencl.reserve_id_t* @sub_group_reserve_read_pipe(%opencl.pipe_t* %{{.*}}, i32 {{.*}})
+  reserve_id_t rid = sub_group_reserve_read_pipe(p, 2);
+  // CHECK: call void @sub_group_commit_read_pipe(%opencl.pipe_t* %{{.*}}, %opencl.reserve_id_t* %{{.*}})
+  sub_group_commit_read_pipe(p, rid);
+}
+
+void test6(write_only pipe int p, global int *ptr) {
+  // CHECK: call %opencl.reserve_id_t* @sub_group_reserve_write_pipe(%opencl.pipe_t* %{{.*}}, i32 {{.*}})
+  reserve_id_t rid = sub_group_reserve_write_pipe(p, 2);
+  // CHECK: call void @sub_group_commit_write_pipe(%opencl.pipe_t* %{{.*}}, %opencl.reserve_id_t* %{{.*}})
+  sub_group_commit_write_pipe(p, rid);
+}
+
+void test7(write_only pipe int p, global int *ptr) {
+  // CHECK: call i32 @get_pipe_num_packets(%opencl.pipe_t* %{{.*}})
+  *ptr = get_pipe_num_packets(p);
+  // CHECK: call i32 @get_pipe_max_packets(%opencl.pipe_t* %{{.*}})
+  *ptr = get_pipe_max_packets(p);
+}

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

+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0
+
+void test1(read_only pipe int p, global int* ptr){
+  int tmp;
+  reserve_id_t rid;
+
+  // read/write_pipe
+  read_pipe(tmp, p);    // expected-error {{first argument to read_pipe must be a pipe type}}
+  read_pipe(p);   // expected-error {{invalid number of arguments to function: read_pipe}}
+  read_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function read_pipe (expecting: 'reserve_id_t')}}
+  read_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function read_pipe (expecting: 'unsigned int')}}
+  read_pipe(p, tmp);   // expected-error {{invalid argument type to function read_pipe (expecting: 'int *')}}
+  write_pipe(p, ptr);    // expected-error {{invalid pipe access modifier (expecting write_only)}}
+  write_pipe(p, rid, tmp, ptr);    // expected-error {{invalid pipe access modifier (expecting write_only)}}
+
+  // reserve_read/write_pipe
+  reserve_read_pipe(p, ptr);    // expected-error{{invalid argument type to function reserve_read_pipe (expecting: 'unsigned int')}}
+  work_group_reserve_read_pipe(tmp, tmp);    // expected-error{{first argument to work_group_reserve_read_pipe must be a pipe type}}
+  sub_group_reserve_write_pipe(p, tmp);    // expected-error{{invalid pipe access modifier (expecting write_only)}}
+
+  // commit_read/write_pipe
+  commit_read_pipe(tmp, rid);    // expected-error{{first argument to commit_read_pipe must be a pipe type}}
+  work_group_commit_read_pipe(p, tmp);    // expected-error{{invalid argument type to function work_group_commit_read_pipe (expecting: 'reserve_id_t')}}
+  sub_group_commit_write_pipe(p, tmp);    // expected-error{{nvalid pipe access modifier (expecting write_only)}}
+}
+
+void test2(write_only pipe int p, global int* ptr){
+  int tmp;
+  reserve_id_t rid;
+
+  // read/write_pipe
+  write_pipe(tmp, p);    // expected-error {{first argument to write_pipe must be a pipe type}}
+  write_pipe(p);   // expected-error {{invalid number of arguments to function: write_pipe}}
+  write_pipe(p, tmp, tmp, ptr);   // expected-error {{invalid argument type to function write_pipe (expecting: 'reserve_id_t')}}
+  write_pipe(p, rid, rid, ptr);   // expected-error {{invalid argument type to function write_pipe (expecting: 'unsigned int')}}
+  write_pipe(p, tmp);   // expected-error {{invalid argument type to function write_pipe (expecting: 'int *')}}
+  read_pipe(p, ptr);    // expected-error {{invalid pipe access modifier (expecting read_only)}}
+  read_pipe(p, rid, tmp, ptr);    // expected-error {{invalid pipe access modifier (expecting read_only)}}
+
+  // reserve_read/write_pipe
+  reserve_write_pipe(p, ptr);    // expected-error{{invalid argument type to function reserve_write_pipe (expecting: 'unsigned int')}}
+  work_group_reserve_write_pipe(tmp, tmp);    // expected-error{{first argument to work_group_reserve_write_pipe must be a pipe type}}
+  sub_group_reserve_read_pipe(p, tmp);    // expected-error{{invalid pipe access modifier (expecting read_only)}}
+
+  // commit_read/write_pipe
+  commit_write_pipe(tmp, rid);    // expected-error{{first argument to commit_write_pipe must be a pipe type}}
+  work_group_commit_write_pipe(p, tmp);    // expected-error{{invalid argument type to function work_group_commit_write_pipe (expecting: 'reserve_id_t')}}
+  sub_group_commit_read_pipe(p, tmp);    // expected-error{{nvalid pipe access modifier (expecting read_only)}}
+}
+
+void test3(){
+  int tmp;
+  get_pipe_num_packets(tmp);    // expected-error {{first argument to get_pipe_num_packets must be a pipe type}}
+  get_pipe_max_packets(tmp);    // expected-error {{first argument to get_pipe_max_packets must be a pipe type}}
+}
+