Page MenuHomePhabricator

[OpenCL] Add Sema checks for atomics and implicit declaration
Needs ReviewPublic

Authored by pxli168 on Feb 19 2016, 12:34 AM.

Details

Summary

Add Sema checks for atomics and implicit declaration
This patch is partitioned from http://reviews.llvm.org/D16047

Diff Detail

Event Timeline

pxli168 retitled this revision from to [OpenCL] Add Sema checks for atomics and implicit declaration.
pxli168 updated this object.
pxli168 added a subscriber: cfe-commits.
Anastasia added inline comments.Feb 19 2016, 12:01 PM
lib/Sema/SemaDecl.cpp
3900

I am still not convinced about this change? Could you give reference to spec or an example? I don't understand why you are trying to change default C behavior.

7283

Why this code?

test/Parser/opencl-atomics-cl20.cl
71

Please remove the macro here as it doesn't have any functionality.

This message seems wrong. Why is it restricted to variables in global address space?

We have similar diagnostic err_atomic_init_constant. Could you reuse it, may be with help of %select?

Anastasia added inline comments.Feb 19 2016, 12:22 PM
lib/Sema/SemaDecl.cpp
11504

Could you add spec reference please!

It seems this patch is useless.
The spec does not tell about implicit declaration of function, but now clang with -triple spir will output err if there is implicit declaration of function.
I have read about spir and opencl spec but could not find anything talk about that.
If this is a clang bug then this patch is useless.

What do you think?

lib/Sema/SemaDecl.cpp
3900

Just removed.

7283

Removed.

Anastasia edited edge metadata.Feb 23 2016, 8:25 AM

I agree there seems to be nothing specifically on this topic in OpenCL spec. However, I wouldn't modify Clang and rely on its default behavior:

  1. In C99 gives a warning
  2. For some targets set up in a special way (i.e. SPIR) gives an error
lib/Sema/SemaInit.cpp
6156

Actually I don't see any statement in spec that forbids initialization of atomic variables inside the functions. It just says that ATOMIC_VAR_INIT can only be used with global variables, but you are not checking that here.

It now gives a warning for both C99 and OpenCL.
But for target SPIR it gives an err only because it treat unprototyped function as varidic function:

/// \brief Checks whether the given calling convention supports variadic
/// calls. Unprototyped calls also use the variadic call rules.
inline bool sthiupportsVariadicCall(CallingConv CC) {...}

and in OpenCL s6.9.e it says

e. Variadic macros and functions with the exception of printf and enqueue_kernel are not supported.

If SPIR is followed this rule, then OpenCL should also give a err. But OpenCL gives only a warning.
I could not find where the following referenced

Unprototyped calls also use the variadic call rules.

What do you think? Should we asked about if implicit declaration of function is allowed in OpenCL, and should we treat these function as variadic functions?

Yes, I think it deserves clarification. Could you submit a bug to Khronos then?

Hi Anastasia,
https://cvs.khronos.org/bugzilla/show_bug.cgi?id=15603
Now we got clarify from khronos. I will continue on this patch.
BTW, https://cvs.khronos.org/bugzilla/show_bug.cgi?id=15599 is also fixed in the same revision.
Thanks
Xiuli

@pxli168, do you still plan to update this? I think the implicit declarations would be quite useful to have.

Yes, I found although the khronos have make a clarify with implicit declarations but they sames to be useful with some program link. And I found some test case about link could not pass by this reason.
But how should we handle the new added spec?

Yes, I found although the khronos have make a clarify with implicit declarations but they sames to be useful with some program link. And I found some test case about link could not pass by this reason.
But how should we handle the new added spec?

Could you add the tests that don't pass to the Khronos bug? I will try to discuss it on the next call.

Thanks!