Refine the type builtin support as the request with
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160201/148637.html
Details
Diff Detail
Event Timeline
LGTM
lib/Sema/SemaChecking.cpp | ||
---|---|---|
343 | should this follow the convention? e.g. |
lib/Sema/SemaChecking.cpp | ||
---|---|---|
343 | right, this will be more clear. |
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 | ||
---|---|---|
294 | Could we use getName() instead? We could then also move this statement after the switch and just set an error flag here. |
lib/Sema/SemaChecking.cpp | ||
---|---|---|
294 | Just to be clear getName() of the attr instead of passing the string (i.e. "read_only"). |
Hi Richard/Anastasia,
I replied in the commit email, and here are some explains:
- Without the space after comma the "//" will be aligned.
- 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.
- 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 | ||
---|---|---|
294 | 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:
- I think I would still add space. You can also reformat other lines. There are only 5 lines above.
- Agree.
- 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?
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.
- Make new indent and leave space for the incoming OpenCL C++.
- Check for the index to see if they are integers.
Could we use getName() instead?
We could then also move this statement after the switch and just set an error flag here.