[PATCH] D15914: [OpenCL] Pipe builtin functions
Xiuli PAN via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 7 21:35:22 PST 2016
pxli168 added a comment.
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?
================
Comment at: include/clang/Basic/Builtins.def:1255
@@ -1254,1 +1254,3 @@
+// OpenCL 2.0 Pipe functions.
+// We need the variadic prototype, since the packet type could be anything.
----------------
Anastasia wrote:
> Could we put reference to spec section?
OK, I will add a reference.
================
Comment at: include/clang/Basic/Builtins.def:1256
@@ +1255,3 @@
+// OpenCL 2.0 Pipe functions.
+// We need the variadic prototype, since the packet type could be anything.
+BUILTIN(read_pipe, "i.", "tn")
----------------
Anastasia wrote:
> Is variadic right here? Should be generic perhaps?
That sounds better.
================
Comment at: include/clang/Basic/Builtins.def:1257
@@ +1256,3 @@
+// We need the variadic prototype, since the packet type could be anything.
+BUILTIN(read_pipe, "i.", "tn")
+BUILTIN(write_pipe, "i.", "tn")
----------------
Anastasia wrote:
> I think it would make sense to have this as LANGBUILTIN as it's only available in OpenCL.
Good idea.
================
Comment at: include/clang/Basic/Builtins.def:1260
@@ +1259,3 @@
+
+BUILTIN(reserve_read_pipe, "i.", "tn")
+BUILTIN(reserve_write_pipe, "i.", "tn")
----------------
Anastasia wrote:
> From reserve_read_pipe onwards the builtins have a fixed number of arguments, and don't need to be variadic.
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.
================
Comment at: lib/CodeGen/CGBuiltin.cpp:2033
@@ +2032,3 @@
+ return RValue::get(
+ Builder.CreateCall(CGM.CreateRuntimeFunction(FTy, Name),
+ {Arg0, Arg1, Arg2, BCast, PacketSize}));
----------------
Anastasia wrote:
> So why do we need to mangle at all since the generated function signature will always have the same parameter types?
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!
================
Comment at: lib/CodeGen/CGOpenCLRuntime.h:55
@@ -50,1 +54,3 @@
};
+class Ocl20Mangler {
+public:
----------------
Anastasia wrote:
> 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.
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?
================
Comment at: lib/Sema/SemaChecking.cpp:267
@@ +266,3 @@
+/// Returns OpenCL access qual.
+static OpenCLImageAccessAttr *getOpenCLImageAcces(const Decl *D) {
+ if (D->hasAttr<OpenCLImageAccessAttr>())
----------------
Anastasia wrote:
> Not sure why we have an image access here?
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.
================
Comment at: lib/Sema/SemaChecking.cpp:268
@@ +267,3 @@
+static OpenCLImageAccessAttr *getOpenCLImageAcces(const Decl *D) {
+ if (D->hasAttr<OpenCLImageAccessAttr>())
+ return D->getAttr<OpenCLImageAccessAttr>();
----------------
Anastasia wrote:
> What happens if access qualifier is not provided?
>
> According to spec it should default to read_only!
Yes, this was handled later when get a null.
We can refine it to a better form later.
================
Comment at: lib/Sema/SemaChecking.cpp:274
@@ +273,3 @@
+/// Returns true if pipe element type is different from the pointer.
+static bool checkOpenCLPipeArg(Sema &S, CallExpr *call) {
+ const Expr *Arg0 = call->getArg(0);
----------------
Anastasia wrote:
> Should we also be checking that read_write is not used with pipe?
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.
================
Comment at: lib/Sema/SemaChecking.cpp:282
@@ +281,3 @@
+ }
+ OpenCLImageAccessAttr *AccessQual =
+ getOpenCLImageAcces(cast<DeclRefExpr>(Arg0)->getDecl());
----------------
Anastasia wrote:
> Should be a generic name rather than image!
Yes, will be done in another commit to make it clearer.
================
Comment at: lib/Sema/SemaChecking.cpp:291
@@ +290,3 @@
+ if (getFunctionName(call).find("read") != StringRef::npos)
+ // if (getFunctionName(call).startswith("read"))
+ isValid = AccessQual == nullptr || AccessQual->isReadOnly();
----------------
Anastasia wrote:
> Remove this commented code
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?
================
Comment at: lib/Sema/SemaChecking.cpp:292
@@ +291,3 @@
+ // if (getFunctionName(call).startswith("read"))
+ isValid = AccessQual == nullptr || AccessQual->isReadOnly();
+ else
----------------
Here we check for null access.
================
Comment at: lib/Sema/SemaChecking.cpp:297
@@ +296,3 @@
+ bool ReadOnly = getFunctionName(call).startswith("read");
+ const char *AM = ReadOnly ? "read_only" : "write_only";
+ S.Diag(Arg0->getLocStart(), diag::err_builtin_pipe_invalid_access_modifier)
----------------
Anastasia wrote:
> Could move this in the above if statement to avoid rechecking.
Ok, good idea.
================
Comment at: lib/Sema/SemaChecking.cpp:307
@@ +306,3 @@
+/// 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);
----------------
Anastasia wrote:
> To adhere the coding standard:
> call -> Call, idx -> Idx
>
> Please, check other places too.
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.
================
Comment at: lib/Sema/SemaChecking.cpp:316
@@ +315,3 @@
+ // the type of pipe element should also be the same.
+ if (!Argidx->getType()->isPointerType() || !ArgTy ||
+ EltTy != ArgTy->getPointeeType().getTypePtr()) {
----------------
Anastasia wrote:
> Aren't first and second checking the same thing?
It seems so, I will delete one.
Thank you.
================
Comment at: lib/Sema/SemaChecking.cpp:378
@@ +377,3 @@
+ }
+ call->setType(S.getASTContext().IntTy);
+ return false;
----------------
Anastasia wrote:
> Do we need setArg as well? Not sure though...
Maybe not, it is declared in the builtin def I think.
================
Comment at: lib/Sema/SemaChecking.cpp:445
@@ +444,3 @@
+ }
+
+ return false;
----------------
Anastasia wrote:
> No call->setType() here?
I is set in switch and case.
================
Comment at: lib/Sema/SemaChecking.cpp:765
@@ +764,3 @@
+ case Builtin::BIsub_group_reserve_write_pipe:
+ // Since those two functions are declared with var args, therefore we need
+ // a semantic check for the argument.
----------------
Anastasia wrote:
> 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.
I think this was for the two function above, I will fix this comment.
http://reviews.llvm.org/D15914
More information about the cfe-commits
mailing list