Page MenuHomePhabricator

[OpenCL] Complete image types support
ClosedPublic

Authored by Anastasia on Mar 2 2016, 10:19 AM.

Details

Summary

Follow up from the earlier discussion via cfe-dev regarding incomplete images support in Clang:
http://lists.llvm.org/pipermail/cfe-dev/2016-February/047187.html

This review is based on older code base (around Clang version 3.5) and is uploaded for quick preview and possible discussions aiming to correct functionality of OpenCL images.

I. Current implementation of images is not conformant to spec in the following points:

  1. It makes no distinction with respect to access qualifiers and therefore allows to use images with different access type interchangeably. The following code would compile just fine:

    void write_image(write_only image2d_t img);

    kernel void foo(read_only image2d_t img) { write_image(img); // Accepted code }

which is disallowed according to s6.13.14.

  1. It discards access qualifier on generated code, which leads to generated code for the above example:

    call void @write_image(%opencl.image2d_t* %img);

In OpenCL2.0 however we can have different calls into write_image with read_only and wite_only images.
Also generally following compiler steps have no easy way to take different path depending on the image access: linking to the right implementation of image types, performing IR opts and backend codegen differently.

  1. Image types are language keywords and can't be redeclared s6.1.9, which can happen currently as they are just typedef names.
  1. Default access qualifier read_only is to be added if not provided explicitly.

II. This patch corrects the above points as follows:

  1. All images are encapsulated into a separate .def file that is inserted in different points where image handling is required. This avoid a lot of code repetition as all images are handled the same way in the code with no distinction of their exact type.
  1. The Cartesian product of image types and image access qualifiers is added to the builtin types. This simplifies a lot handling of access type mismatch as no operations are allowed by default on distinct Builtin types. Also spec intended access qualifier as special type qualifier that are combined with an image type to form a distinct type (see statement above - images can't be created w/o access qualifiers).
  1. Improves testing of images in Clang.

This patch would require rebasing, fixing formatting, and also addition of OpenCL2.0 images and pipes, to be committed.

Diff Detail

Repository
rL LLVM

Event Timeline

Anastasia updated this revision to Diff 49618.Mar 2 2016, 10:19 AM
Anastasia retitled this revision from to [OpenCL] Complete image types support.
Anastasia updated this object.
Anastasia added a reviewer: bader.
bader edited edge metadata.Mar 10 2016, 8:03 AM

If there are no objections to adopt this approach I can start working on rebasing that patch to the ToT.

include/clang/AST/OpenCLImageTypes.def
39 ↗(On Diff #49618)

Minor comment: any image access qualifier can be applied to any image type, so I think it's better to list all image types once to avoid duplication.

Anastasia added inline comments.Mar 11 2016, 8:52 AM
include/clang/AST/OpenCLImageTypes.def
39 ↗(On Diff #49618)

Sure, we could try that. Although, I think there are places where this file is included needing images without an access qualifier using GENERIC_IMAGE_TYPE macro.

For example in include/clang/Basic/TokenKinds.def

Anastasia updated this revision to Diff 51303.Mar 22 2016, 10:57 AM
Anastasia edited edge metadata.

Thanks to Aleksey Bader for rebasing this patch to ToT and fixing some tests issues!

mgrang added a subscriber: mgrang.Mar 23 2016, 12:02 PM
mgrang added inline comments.
include/clang/AST/ASTContext.h
903 ↗(On Diff #51303)

remove extra spacing in front of the \

bader added inline comments.Apr 5 2016, 9:11 AM
include/clang/AST/ASTContext.h
903 ↗(On Diff #51303)

Sorry for delay...
I used clang-format tool to format that code. I expect it to be in agreement with LLVM coding style guide.
Did I miss something or it's clang-format bug?

Regarding, extending this approach for OpenCL pipe types too. I was thinking we could change current implementation to have ReadPipeType and WritePipeType. They can both be derived from PipeType that we already have now (we can make it an abstract class to avoid its instantiation?).

Similarly to images, since read and write pipes will be mapped to different Clang types, we won't need any extra semantical checking but just need to add extra code in CodeGen of pipe type and builtins to accept two separate types for read only and write only cases.

Would this make sense?

include/clang/AST/ASTContext.h
903 ↗(On Diff #51303)

I would have thought clang-format should be fine.

But it does look a bit weird here. There are other places in this patch that have the same formatting.

I can't find anything relevant in coding style description:
http://llvm.org/docs/CodingStandards.html#source-code-width

bader added a comment.Apr 6 2016, 11:37 AM

Regarding, extending this approach for OpenCL pipe types too. I was thinking we could change current implementation to have ReadPipeType and WritePipeType. They can both be derived from PipeType that we already have now (we can make it an abstract class to avoid its instantiation?).

Similarly to images, since read and write pipes will be mapped to different Clang types, we won't need any extra semantical checking but just need to add extra code in CodeGen of pipe type and builtins to accept two separate types for read only and write only cases.

Would this make sense?

Sure. Do you want me to add it here or it's okay if we fix pipes in a separate patch?

include/clang/AST/ASTContext.h
903 ↗(On Diff #51303)

I'll remove extra spacing.

Regarding, extending this approach for OpenCL pipe types too. I was thinking we could change current implementation to have ReadPipeType and WritePipeType. They can both be derived from PipeType that we already have now (we can make it an abstract class to avoid its instantiation?).

Similarly to images, since read and write pipes will be mapped to different Clang types, we won't need any extra semantical checking but just need to add extra code in CodeGen of pipe type and builtins to accept two separate types for read only and write only cases.

Would this make sense?

Sure. Do you want me to add it here or it's okay if we fix pipes in a separate patch?

Yes, I think it's better to go in a separate commit, not to complicate this one too much. Also since there are not many comment here, I think we should try to commit it ASAP otherwise rebasing would be an issue.

bader added a comment.Apr 7 2016, 11:29 AM

Yes, I think it's better to go in a separate commit, not to complicate this one too much. Also since there are not many comment here, I think we should try to commit it ASAP otherwise rebasing would be an issue.

I can commit it tomorrow if there are no other comments.

This revision was automatically updated to reflect the committed changes.
bader added a comment.Apr 8 2016, 6:46 AM

Yes, I think it's better to go in a separate commit, not to complicate this one too much. Also since there are not many comment here, I think we should try to commit it ASAP otherwise rebasing would be an issue.

I can commit it tomorrow if there are no other comments.

Committed @265783