Page MenuHomePhabricator

[OpenCL] Fix support for cl_khr_mipmap_image_writes
ClosedPublic

Authored by AlexeySotkin on Dec 13 2019, 4:14 AM.

Diff Detail

Event Timeline

AlexeySotkin created this revision.Dec 13 2019, 4:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2019, 4:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kpet added a subscriber: kpet.Dec 13 2019, 4:21 AM
AlexeySotkin edited the summary of this revision. (Show Details)Dec 13 2019, 4:29 AM
asavonic added a comment.EditedDec 13 2019, 4:45 AM

What about get_image_num_mip_levels functions defined in the extension specification?

Edit: I mean, should the get_image_num_mip_levels(write_only img) function be only available if cl_khr_mipmap_image_writes extension is supported, or cl_khr_mipmap_image is enough?

clang/lib/Headers/opencl-c.h
14777

Shouldn't "color" be renamed to "depth" here as well?

Anastasia added inline comments.Dec 16 2019, 11:56 AM
clang/lib/Headers/opencl-c.h
14686

Do we actually need pragma for this extension? I.e. does it need to activate any special mode in the compiler?

What about get_image_num_mip_levels functions defined in the extension specification?

Edit: I mean, should the get_image_num_mip_levels(write_only img) function be only available if cl_khr_mipmap_image_writes extension is supported, or cl_khr_mipmap_image is enough?

I think cl_khr_mipmap_image is enough, because "the cl_khr_mipmap_image_writes extension adds built-in functions that can be used to write a mip-mapped image", while get_image_num_mip_levels(write_only img) only retrieve a property of the image.

AlexeySotkin marked an inline comment as done.Dec 18 2019, 3:37 AM
AlexeySotkin added inline comments.
clang/lib/Headers/opencl-c.h
14686

I'm not aware of any special mode required for this extension. These begin/end pragma lines were added to disable the extension by default as it is required by the OpenCL extension spec. Is there any other mechanism which should be used for this purpose?
Probably, we should do the same for cl_khr_mipmap_image(and maybe others?), because with the current compiler, built-ins from this extension can be compiled successfully even if #pragma OPENCL EXTENSION cl_khr_mipmap_image : disable is specified. See https://godbolt.org/z/fNEWuG

Rename color to depth

AlexeySotkin marked an inline comment as done.Dec 19 2019, 12:59 AM
Anastasia added inline comments.Dec 27 2019, 8:06 AM
clang/lib/Headers/opencl-c.h
14686

What I am saying is that pragma is only typically used to activate some special mode in the compiler. If we don't need to activate anything perhaps we don't need to add pragma at all? It simplifies compiler and application code too. Would regular include mechanisms be enough to guard the availability of those functions?

Maybe this thread will help to understand the topic better: https://github.com/KhronosGroup/OpenCL-Docs/issues/82

AlexeySotkin marked 2 inline comments as done.
AlexeySotkin added inline comments.
clang/lib/Headers/opencl-c.h
14686

I think you are right. I have removed the pragma.

Anastasia accepted this revision.Jan 28 2020, 9:41 AM

LGTM! Thanks!

clang/lib/Headers/opencl-c.h
14686

I think we probably should get rid of most of extensions in clang/include/clang/Basic/OpenCLExtensions.def and just try to implement the extensions macro using target specific header.

But it doesn't belong to your change and it's probably too big of the change anyway. Just a thought. :)

This revision is now accepted and ready to land.Jan 28 2020, 9:41 AM

Adding @svenvh mainly to check if any fix is needed for the TableGen BIFs too?

Adding @svenvh mainly to check if any fix is needed for the TableGen BIFs too?

Yes I believe we should also update clang/lib/Sema/OpenCLBuiltins.td.

This revision was automatically updated to reflect the committed changes.
AlexeySotkin marked an inline comment as done.
svenvh added a comment.Feb 5 2020, 8:07 AM

I have updated the TableGen OpenCL builtin definitions accordingly in 91b3083aecd ("[OpenCL] Fix tblgen support for cl_khr_mipmap_image_writes", 2020-02-05).

Is there any chance to get this commit cherry-picked to 10.x branch?