Page MenuHomePhabricator

[OpenCL] Add const, volatile and pointer handling for builtin functions
ClosedPublic

Authored by svenvh on Jun 17 2019, 9:56 AM.

Details

Summary

Volatile, const and pointer types were previously available, but not
working. This patch adds handling for OpenCL builtin functions.

Some atomic functions are added to show examples of use of
the PointerType and the VolatileType.

This depends on:
-First, splitting opencl-c.h file: https://reviews.llvm.org/D63256/new/
-Second, Adding generic types: https://reviews.llvm.org/D63434

Diff Detail

Repository
rL LLVM

Event Timeline

Pierre created this revision.Jun 17 2019, 9:56 AM
Pierre updated this revision to Diff 205397.Jun 18 2019, 10:42 AM
Pierre added projects: Restricted Project, Restricted Project.

Add context lines.

Anastasia added inline comments.Jun 21 2019, 6:54 AM
clang/lib/Sema/OpenCLBuiltins.td
294 ↗(On Diff #205397)

This patch also seems to be adding atomics?

Pierre updated this revision to Diff 206700.Jun 26 2019, 9:35 AM
Pierre edited the summary of this revision. (Show Details)
Pierre updated this revision to Diff 206702.Jun 26 2019, 9:46 AM

Add ConstType and VolatileType in a comment

Anastasia added inline comments.Jul 1 2019, 9:16 AM
clang/lib/Sema/OpenCLBuiltins.td
64 ↗(On Diff #206702)

We seem to have PointerType twice now...

99 ↗(On Diff #206702)

Why changing this comment?

112 ↗(On Diff #206702)

I would suggest
CVQualType<Type _Ty, bit IsConst, bit IsVolatile>
just to avoid code duplication.

Btw don't you need to set QualType too?

245 ↗(On Diff #206702)

Do you need this here?

clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
23 ↗(On Diff #206702)

Can we test const too?

If you remove volatile does clang give diagnostic?

Pierre updated this revision to Diff 207494.Jul 2 2019, 2:58 AM
Pierre marked 4 inline comments as done.

Typo and correct the order of fields in the Type class (.td file)

clang/lib/Sema/OpenCLBuiltins.td
99 ↗(On Diff #206702)

I wanted to be consistent with the colon, I've put back the "float*"

112 ↗(On Diff #206702)

I would suggest
CVQualType<Type _Ty, bit IsConst, bit IsVolatile>
just to avoid code duplication.

I thought creating a class for these specific cases would be clearer than having a bitfield.
Also, there are not a lot of functions using const/ volatile types, creating a class for these specific cases avoids to have fields to fill with blank values. This can be avoided with default initialisation though (and not setting these fields).

Btw don't you need to set QualType too?

The name is set by giving arguments to the parent class line 101. All this let statements are here to be sure we are taking the values of the type _Ty given as argument, and not setting the fields to the default values of the Type class.

245 ↗(On Diff #206702)

This needs to be after the definition of all the types (char_t, int_t, etc), and before the definitions of the GenericType instances (and before the definition of the builtin functions)

clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl
23 ↗(On Diff #206702)

Can we test const too?

I can add a test, but for this I need to find a builtin function using a const qualifier and add it in this patch aswell (cf the atomic functions added in the .td file). I'm ok to add some more functions if you agree.

If you remove volatile does clang give diagnostic?

Indeed, I though I remembered there was a warning emitted when the volatile/ const qualifier was not present. Warnings are only emitted in one case (see the table below).

argument given has \ function expectsscalar no qualifierscalar const/volatile arg
scalar no qualifierOKNo warning
scalar const/volatileNo WarningOK

and

argument given has \ function expectspointer no qualifierpointer to const/volatile arg
pointer to const/volatileOKNo warning
pointer to const/volatileWarningOK

So no diagnostic would be emitted in this test, whether we give a const/ volatile or not to atom_add.

Pierre updated this revision to Diff 207802.Jul 3 2019, 8:10 AM

Rebase from the patch adding GenTypes.

Pierre updated this revision to Diff 207805.Jul 3 2019, 8:36 AM

Reorder fields in the Type structure in the .td file

Pierre updated this revision to Diff 208344.Jul 8 2019, 3:11 AM
svenvh commandeered this revision.Wed, Jul 31, 7:43 AM
svenvh updated this revision to Diff 212582.
svenvh edited reviewers, added: Pierre; removed: svenvh.
  • Factor out .td def renames into separate patch (already pushed).
  • Add definitions for some atomic and asynchronous builtins to make use of the new functionality.
  • Fix formatting.
svenvh updated this revision to Diff 215887.Mon, Aug 19, 7:12 AM

Rebased onto latest master.

Anastasia accepted this revision.Mon, Aug 19, 7:48 AM

LGTM! Thanks!

clang/lib/Sema/OpenCLBuiltins.td
91 ↗(On Diff #215887)

shouldn't this be always false?

94 ↗(On Diff #215887)

this should be none

107 ↗(On Diff #215887)

this should be none

335 ↗(On Diff #215887)

At least the message in the commit should reflect that we are also adding new BIFs.

This revision is now accepted and ready to land.Mon, Aug 19, 7:48 AM
svenvh marked 2 inline comments as done.Mon, Aug 19, 8:48 AM
svenvh added inline comments.
clang/lib/Sema/OpenCLBuiltins.td
91 ↗(On Diff #215887)

In theory yes, but I think I'd rather propagate the value instead of silently dropping it.

94 ↗(On Diff #215887)

As above, I feel it's safer to propagate the value instead of silently dropping it.

This revision was automatically updated to reflect the committed changes.