Page MenuHomePhabricator

[OpenCL] Add predefined macros.
ClosedPublic

Authored by yaxunl on Apr 13 2016, 1:17 PM.

Details

Summary

OpenCL spec requires __OPENCL_C_VERSION__ to be defined based on -cl-std option. This patch implements that.

The patch also defines __FAST_RELAXED_MATH__ based on -cl-fast-relaxed-math option.

Also fixed a test using -std=c99 for OpenCL program. Limit allowed language standard of OpenCL to be OpenCL standards.

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl updated this revision to Diff 53606.Apr 13 2016, 1:17 PM
yaxunl retitled this revision from to [OpenCL] Add predefined macros..
yaxunl updated this object.
yaxunl added reviewers: Anastasia, bader, pxli168.
yaxunl updated this object.
yaxunl added subscribers: tstellarAMD, cfe-commits.
pxli168 added inline comments.Apr 13 2016, 8:21 PM
lib/Frontend/InitPreprocessor.cpp
421 ↗(On Diff #53606)

What is 0 stand for as OpenCLVersion, it seems the default OpenCLVersion if we did not pass any -cl-std=CLXX is 100.
So maybe we did not need this case 0:

439 ↗(On Diff #53606)

These macros maybe need for all cl version, and in the header we should compare OPENCL_C_VERSION with CL_VERSION_2_0 instead of the integer 200 in the header of http://reviews.llvm.org/D18369?

yaxunl added inline comments.Apr 14 2016, 8:58 AM
lib/Frontend/InitPreprocessor.cpp
421 ↗(On Diff #53606)

There is a test test/Frontend/stdlang.c:

// RUN: %clang_cc1 -x cl -std=c99 -DOPENCL %s

With this test, LangOpts.OpenCL is 1 but LangOpts.OpenCLVersion is 0.

If we all agree that this test is invalid and -x cl can only be compiled with OpenCL language standard and the default one is OpenCL 1.0, I can fix that test first and then remove case 0 in this switch.

439 ↗(On Diff #53606)

Each OpenCL version only defines some of these macros by spec.

In the header file __OPENCL_C_VERSION__ can compare with 200 since the spec defines the integer value for __OPENCL_C_VERSION__. Comparing with CL_VERSION_2_0 requires checking CL_VERSION_2_0 is available first. I think probably I can define a macro

#define _OPENCL20_AND_ABOVE defined(__OPENCL_C_VERSION__) and defined(CL_VERSION_2_0) and __OPENCL_C_VERSION__ >= 200

and then use this macro for conditioning 2.0 specific functions.

Anastasia added inline comments.Apr 15 2016, 4:59 AM
lib/Frontend/InitPreprocessor.cpp
426 ↗(On Diff #53606)

So why we can't use unified OPENCL_C_VERSION?

438 ↗(On Diff #53606)

I am not sure we should add this conditionally though. If you want to compile CL code like this (forcing pointer to point to private AS for all CL versions the code is compiled for):

#if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
private void* priv_ptr;
#else
void* priv_ptr;
#endif

This code would fail with -cl-sdt=CL1.1 (because CL_VERSION_2_0 is no longer defined), but the purpose of having version macros is to be able to condition on the passed CL version in the particular compilation run to avoid the compiler failure/miscompilation. This way we provide a way to select the right CL code for all versions the code is being compiled (if such selection is needed) to allow portability of CL code among OpenCL version.

yaxunl marked 6 inline comments as done.Apr 15 2016, 7:30 AM
yaxunl added inline comments.
lib/Frontend/InitPreprocessor.cpp
426 ↗(On Diff #53606)

__OPENCL_C_VERSION__ is not defined in OpenCL spec v1.0 and 1.1.

438 ↗(On Diff #53606)

CL_VERSION_x_y is only defined in OpenCL spec version equal or above x.y.

We can use macros like

#if defined(__OPENCL_C_VERSION__) && defined(CL_VERSION_2_0) && __OPENCL_C_VERSION__ >= CL_VERSION_2_0
#define _CL20_AND_ABOVE 1
#endif
439 ↗(On Diff #53606)

should be

#if defined(__OPENCL_C_VERSION__) && defined(CL_VERSION_2_0) && __OPENCL_C_VERSION__ >= CL_VERSION_2_0
#define _CL20_AND_ABOVE 1
#endif
Anastasia added inline comments.Apr 19 2016, 9:48 AM
lib/Frontend/InitPreprocessor.cpp
439 ↗(On Diff #53606)

Where do you plan to add this code?

My point is that the code is a bit complicated and messy with this checks and on the other hand it's kind of logical that compiler supporting all CL version would also accept all CL_VERSION_X_Y macros irrespectively from the passed -cl-std flag... I don't think spec is being that explicit on how CL2.0 compliant compiler should enable those macros when earlier versions of -cl-std are being passed.

yaxunl marked 7 inline comments as done.Apr 19 2016, 10:16 AM
yaxunl added inline comments.
lib/Frontend/InitPreprocessor.cpp
421 ↗(On Diff #53606)

Anastasia,

What's your opinion on this? Should we limit the allowed language standard for cl to OpenCL standard only? Should I diagnose invalid language standard and also add a lit test for that?

Thanks.

439 ↗(On Diff #53606)

OK I will add these macros.

yaxunl updated this revision to Diff 54243.Apr 19 2016, 12:41 PM
yaxunl updated this object.
yaxunl edited edge metadata.
yaxunl marked an inline comment as done.

Added OPENCL_C_VERSION and CL_VERSION_X_Y for all OpenCL versions.

Also fixed the test using -std=c99 for OpenCL and limit allowed language standard for OpenCL.

I think an OpenCL program by definition should follow certain OpenCL language standard.

Anastasia added inline comments.Apr 20 2016, 9:30 AM
lib/Frontend/InitPreprocessor.cpp
421 ↗(On Diff #54243)

Yes, perfectly makes sense. I don't think passing C std flag should be allowed since OpenCL is based strictly on C99 and use of other versions are not possible anyways.

Nice cleanup!

439 ↗(On Diff #54243)

Could you update the test please.

test/Frontend/stdlang.c
10 ↗(On Diff #54243)

I think expected-no-diagnostics is only used if -verify is passed.

yaxunl updated this revision to Diff 54404.Apr 20 2016, 12:52 PM

Fix test/preprocessor/predefined_macros.c.

Merge test/frontend/std.cl into langstd.c since they are similar.

Anastasia added inline comments.Apr 21 2016, 10:45 AM
lib/Frontend/InitPreprocessor.cpp
418 ↗(On Diff #54404)

CLANG_OPENCL_C_VERSION -> OPENCL_C_VERSION

test/Frontend/stdlang.c
11 ↗(On Diff #54404)

I still think that "// expected-no-diagnostics" is not needed here as we don't pass -verify.

yaxunl updated this revision to Diff 54552.Apr 21 2016, 11:20 AM

Revised as Anastasia suggested.

Anastasia added inline comments.Apr 26 2016, 7:38 AM
test/Preprocessor/predefined-macros.c
180 ↗(On Diff #54552)

Could we also add CHECK-NOT for all the above cases to make sure FAST_RELAXED_MATH doesn't appear there.

yaxunl updated this revision to Diff 55016.Apr 26 2016, 8:52 AM
yaxunl marked 4 inline comments as done.

Add CHECK-NOT for __FAST_RELAXED_MATH__.

Anastasia accepted this revision.Apr 26 2016, 9:20 AM
Anastasia edited edge metadata.

LGTM! Thanks!

This revision is now accepted and ready to land.Apr 26 2016, 9:20 AM
This revision was automatically updated to reflect the committed changes.