Page MenuHomePhabricator

[OpenCL] opencl-c.h: remove arg names from atomics
ClosedPublic

Authored by svenvh on Feb 11 2022, 10:01 AM.

Details

Summary

This simplifies completeness comparisons against OpenCLBuiltins.td and
also makes the header no longer "claim" the identifiers "success",
"failure", "desired", "value" (such that you can compile with -Dvalue=...
when including the header for example, which currently breaks parsing
of the header).

I made this patch to be able to do some completeness comparisons locally
for D119420, and thought it might be worth committing anyway.

This is a big patch and it only touches the OpenCL 2 atomics for now. I
wonder if we should remove argument names from the other builtins too?
I think it would help unifying the header and tablegen approaches: if we
gradually move the header into some canonical form, we might be able
to eventually replace it with a tablegen-ed header, while being able to
easily confirm equivalence.

Diff Detail

Event Timeline

svenvh created this revision.Feb 11 2022, 10:01 AM
svenvh requested review of this revision.Feb 11 2022, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2022, 10:01 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

also makes the header no longer "claim" the identifiers "success",
"failure", "desired", "value" (such that you can compile with -Dvalue=...
when including the header for example, which currently breaks parsing
of the header).

I don't get what you mean by this. :)

This is a big patch and it only touches the OpenCL 2 atomics for now. I
wonder if we should remove argument names from the other builtins too?
I think it would help unifying the header and tablegen approaches: if we
gradually move the header into some canonical form, we might be able
to eventually replace it with a tablegen-ed header, while being able to
easily confirm equivalence.

The only drawback I see if that we will lose the history a bit in "git blame" but:

  • The biggest part of the header was committed in one block anyway
  • We should drive toward deprecation of the header development and moving it

towards testing only feature.
So I think this is totally justifiable since it is a good step towards the end goal.

Anastasia accepted this revision.Feb 15 2022, 4:26 AM

LGTM! Thanks

This revision is now accepted and ready to land.Feb 15 2022, 4:26 AM

also makes the header no longer "claim" the identifiers "success",
"failure", "desired", "value" (such that you can compile with -Dvalue=...
when including the header for example, which currently breaks parsing
of the header).

I don't get what you mean by this. :)

Compiling a CL source file with e.g. clang -cl-std=CL2.0 -Xclang -finclude-default-header -cl-no-stdinc -Dvalue=1 clang/test/CodeGenOpenCL/as_type.cl gives lots of errors such as the following, because defining value as a macro (which is not a reserved identifier as far as I'm aware) collides with the argument names in the header:

In file included from <built-in>:1:
lib/clang/15.0.0/include/opencl-c.h:13277:58: error: expected ')'
void __ovld atomic_init(volatile atomic_int *object, int value);
                                                         ^
<command line>:1:15: note: expanded from here
#define value 1

This is a big patch and it only touches the OpenCL 2 atomics for now. I
wonder if we should remove argument names from the other builtins too?
I think it would help unifying the header and tablegen approaches: if we
gradually move the header into some canonical form, we might be able
to eventually replace it with a tablegen-ed header, while being able to
easily confirm equivalence.

The only drawback I see if that we will lose the history a bit in "git blame" but:

Slight nuance: we will not lose any history, but I understand your concern: someone needs to look through this commit to see the previous commit that touched the code.

If there are no objections to removing all argument names from the header, I'll try to prepare patches for doing so.

also makes the header no longer "claim" the identifiers "success",
"failure", "desired", "value" (such that you can compile with -Dvalue=...
when including the header for example, which currently breaks parsing
of the header).

I don't get what you mean by this. :)

Compiling a CL source file with e.g. clang -cl-std=CL2.0 -Xclang -finclude-default-header -cl-no-stdinc -Dvalue=1 clang/test/CodeGenOpenCL/as_type.cl gives lots of errors such as the following, because defining value as a macro (which is not a reserved identifier as far as I'm aware) collides with the argument names in the header:

In file included from <built-in>:1:
lib/clang/15.0.0/include/opencl-c.h:13277:58: error: expected ')'
void __ovld atomic_init(volatile atomic_int *object, int value);
                                                         ^
<command line>:1:15: note: expanded from here
#define value 1

Oh, I see. Thanks! This is bad indeed!!!

This is a big patch and it only touches the OpenCL 2 atomics for now. I
wonder if we should remove argument names from the other builtins too?
I think it would help unifying the header and tablegen approaches: if we
gradually move the header into some canonical form, we might be able
to eventually replace it with a tablegen-ed header, while being able to
easily confirm equivalence.

The only drawback I see if that we will lose the history a bit in "git blame" but:

Slight nuance: we will not lose any history, but I understand your concern: someone needs to look through this commit to see the previous commit that touched the code.

Yeah indeed, we don't lose it, but just make it a bit more complicated to look up.

If there are no objections to removing all argument names from the header, I'll try to prepare patches for doing so.

No, it seems rather good to do since it is fixing a bug with the preprocessor macros that you have highlighted.

This revision was landed with ongoing or failed builds.Feb 21 2022, 3:29 AM
This revision was automatically updated to reflect the committed changes.