Page MenuHomePhabricator

[OpenCL] Add extension cl_khr_mipmap_image to clang
ClosedPublic

Authored by ashi1 on Jul 21 2016, 11:26 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ashi1 updated this revision to Diff 64930.Jul 21 2016, 11:26 AM
ashi1 retitled this revision from to [OpenCL] Add extension cl_khr_mipmap_image to clang.
ashi1 updated this object.
ashi1 added reviewers: yaxunl, Anastasia.
ashi1 set the repository for this revision to rL LLVM.
ashi1 added a subscriber: cfe-commits.
yaxunl edited edge metadata.Jul 21 2016, 11:44 AM

Need to add a test to Misc/amdgcn.languageOptsOpenCL.cl and SemaOpenCL/extension-version.cl

yaxunl added inline comments.Jul 21 2016, 11:46 AM
include/clang/Basic/OpenCLExtensions.def
73 ↗(On Diff #64930)

Better to follow the alphabetical order.

ashi1 updated this revision to Diff 65118.Jul 22 2016, 11:48 AM
ashi1 edited edge metadata.
ashi1 marked an inline comment as done.

Revised based on Sam's comments. Added to tests Misc\amdgcn.languageOptsOpenCL.cl and SemaOpenCL\extension-version.cl

yaxunl added inline comments.Jul 22 2016, 12:07 PM
test/Misc/amdgcn.languageOptsOpenCL.cl
190 ↗(On Diff #65118)

we also need to check the else case for __OPENCL_C_VERSION__ < 200 that cl_khr_mipmap_image is defined and no warning msg for #pragma OPENCL EXTENSION cl_khr_mipmap_image: enable

ashi1 updated this revision to Diff 65126.Jul 22 2016, 12:53 PM
ashi1 marked an inline comment as done.

Revised based on Sam's comments.

yaxunl accepted this revision.Jul 22 2016, 1:14 PM
yaxunl edited edge metadata.

LGTM. Thanks!

This revision is now accepted and ready to land.Jul 22 2016, 1:14 PM
Anastasia added inline comments.Jul 25 2016, 9:35 AM
test/Misc/amdgcn.languageOptsOpenCL.cl
188 ↗(On Diff #65126)

Can you move this error message down instead of adding 6 lines offset.
Also the extension seems to be set by default although it's hard to see without the full diff. So why do you get this warning at all?

test/SemaOpenCL/extension-version.cl
228 ↗(On Diff #65126)

Can you move this error message down instead of adding 6 lines offset.

yaxunl added inline comments.Jul 25 2016, 10:33 AM
test/Misc/amdgcn.languageOptsOpenCL.cl
188 ↗(On Diff #65126)

This warning is only for opencl version < 200. The extension is supported by OpenCL 2.0 and above only.

ashi1 added inline comments.Jul 25 2016, 10:40 AM
test/Misc/amdgcn.languageOptsOpenCL.cl
188 ↗(On Diff #65126)

I've added this error message here because I am following the order inside include/clang/Basic/OpenCLExtensions.def. Seems it is alphabetical there and in same order here. What do you think?

test/SemaOpenCL/extension-version.cl
228 ↗(On Diff #65126)

I've added this error message here because I am following the order inside include/clang/Basic/OpenCLExtensions.def. Seems it is alphabetical there and in same order here. What do you think?

Anastasia added inline comments.Jul 26 2016, 8:27 AM
test/Misc/amdgcn.languageOptsOpenCL.cl
188 ↗(On Diff #65126)

This warning is to line 194 which is an else from:

#if (__OPENCL_C_VERSION__ < 200)

hence OpenCL >= 2.0. There should be no warning in this case?

test/SemaOpenCL/extension-version.cl
228 ↗(On Diff #65126)

Sure that's great! I am just saying could you move the expected-warning line below to where it belongs i.e. line 234 where you are enabling the extension which results in a warning generated.

Anastasia added inline comments.Jul 26 2016, 8:40 AM
test/SemaOpenCL/extension-version.cl
228 ↗(On Diff #65126)

My main problem here is that the check for the warning is too far away from the place that generates it and therefore difficult to understand. If we could restructure this somehow. I think it's also OK to move this line above:

#pragma OPENCL EXTENSION cl_khr_mipmap_image: enable

This test is already hard to read, let's try to simplify it a bit...

ashi1 updated this revision to Diff 65582.Jul 26 2016, 1:19 PM
ashi1 edited edge metadata.

Revised based on Anastasia's comments. Restructured the if statements in test cases.

Anastasia added inline comments.Jul 27 2016, 10:00 AM
test/Misc/amdgcn.languageOptsOpenCL.cl
188 ↗(On Diff #65582)

Looks good! Could you just remove indentation please as it's not common for C macros?

The same for the test below!

ashi1 updated this revision to Diff 65773.Jul 27 2016, 10:48 AM
ashi1 marked an inline comment as done.

Removed indentation as per Anastasia's comments.

Anastasia accepted this revision.Jul 28 2016, 10:31 AM
Anastasia edited edge metadata.

LGTM!

This revision was automatically updated to reflect the committed changes.