This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Refine pipe builtin support
ClosedPublic

Authored by pxli168 on Feb 4 2016, 1:14 AM.

Diff Detail

Event Timeline

pxli168 updated this revision to Diff 46880.Feb 4 2016, 1:14 AM
pxli168 retitled this revision from to [OpenCL] Refine pipe builtin support.
pxli168 updated this object.
pxli168 added a subscriber: cfe-commits.
yaxunl accepted this revision.Feb 4 2016, 10:47 AM
yaxunl edited edge metadata.

LGTM

lib/Sema/SemaChecking.cpp
339–340

should this follow the convention? e.g.
OpenCL v2.0 s6.13.16.2 -

This revision is now accepted and ready to land.Feb 4 2016, 10:47 AM
pxli168 requested a review of this revision.Feb 4 2016, 6:41 PM
pxli168 updated this revision to Diff 46991.
pxli168 edited edge metadata.
pxli168 edited edge metadata.
pxli168 added inline comments.
lib/Sema/SemaChecking.cpp
339–340

right, this will be more clear.

yaxunl accepted this revision.Feb 5 2016, 7:47 AM
yaxunl edited edge metadata.

LGTM. Thanks.

This revision is now accepted and ready to land.Feb 5 2016, 7:47 AM
Anastasia edited edge metadata.Feb 5 2016, 9:44 AM

I think some comments from Richad's feedback are missing, specifically:

  • Missing space after comma.
  • Why are these conversions performed here rather than in Sema?
  • You should attempt to implicitly convert to the desired type here, rather than demanding the right type, to match the normal call semantics. Likewise elsewhere in this patch.

Do you plan to commit them in a separate patch?

Also please add Richard to reviewers list here for the final Ok.

Thanks!

lib/Sema/SemaChecking.cpp
293

Could we use getName() instead?

We could then also move this statement after the switch and just set an error flag here.

Anastasia added inline comments.Feb 5 2016, 9:48 AM
lib/Sema/SemaChecking.cpp
293

Just to be clear getName() of the attr instead of passing the string (i.e. "read_only").

pxli168 requested a review of this revision.Feb 5 2016, 5:55 PM
pxli168 added a reviewer: rsmith.

Hi Richard/Anastasia,

I replied in the commit email, and here are some explains:

  1. Without the space after comma the "//" will be aligned.
  2. We want generic prototypes of the builtin functions, and we generate them in CodeGen. So there will be no use to perform conversion in sema for the function prototype.
  3. We used generic prototypes and varadic function declration, the conversion in CodeGen use cast and won't change the original type for some arguments that maybe used elsewhere. (Also I do not understand very clear about how to convert in Sema)

Thanks
Xiuli

lib/Sema/SemaChecking.cpp
293

Here we need a different access qualifier then we have. I seems to complicated to generated a useless attribute here just for a name.

Related to:

  1. I think I would still add space. You can also reformat other lines. There are only 5 lines above.
  2. Agree.
  3. Feels like may be we should try to see if the passed argument is convertible to the function parameter type.

For example, I see that some builtins in SemaChecking.cpp use DefaultFunctionArrayLvalueConversion to try converting to a pointer type. I am not sure what we could do for other types though especially for the OpenCL types.

@Richard, would you be able to give us more information here:

> +  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()) {

You should attempt to implicitly convert to the desired type here, rather than demanding the right type, to match the normal call semantics. Likewise elsewhere in this patch.

Could you please address number 1 from my previous comment?

Otherwise, I think we should try to proceed quickly here, it will be too hard to merge back in after long delay and also it would be nice to have as many corrections as possible ASAP.

Could we move Richard to subscribers for now, as he can give his feedback later too.

@pekka, do you have any more comments?

pekka.jaaskelainen edited edge metadata.

@pekka, do you have any more comments?

Nope. Looking forward to finally implementing proper pipe support to pocl.

With the future Clang OpenCL patches I appreciate if you keep adding me to the cc list of the reviews, but it might be pointless to wait for my LGTM as I'm still quite a Clang noob, thus that "LGTM" might not have the "weight" it should have :) In other words, I keep monitoring the patches and will yell if I see something that sticks to my eye, but no point in blocking the progress due to possibly slow acks from my side.

This revision is now accepted and ready to land.Feb 23 2016, 8:54 AM

@pekka, do you have any more comments?

Nope. Looking forward to finally implementing proper pipe support to pocl.

With the future Clang OpenCL patches I appreciate if you keep adding me to the cc list of the reviews, but it might be pointless to wait for my LGTM as I'm still quite a Clang noob, thus that "LGTM" might not have the "weight" it should have :) In other words, I keep monitoring the patches and will yell if I see something that sticks to my eye, but no point in blocking the progress due to possibly slow acks from my side.

Sure, no problem! Thanks!

pxli168 removed a reviewer: rsmith.Feb 24 2016, 9:32 PM
pxli168 added a subscriber: rsmith.
pxli168 updated this revision to Diff 49008.Feb 24 2016, 9:57 PM
  1. Make new indent and leave space for the incoming OpenCL C++.
  2. Check for the index to see if they are integers.
yaxunl accepted this revision.Feb 25 2016, 9:04 AM
yaxunl edited edge metadata.

LGTM. Thanks.

Anastasia accepted this revision.Mar 1 2016, 11:09 AM
Anastasia edited edge metadata.

LGTM!

pxli168 updated this revision to Diff 49797.Mar 3 2016, 9:51 PM
pxli168 edited edge metadata.
pxli168 closed this revision.Mar 3 2016, 11:15 PM