This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Accept __attribute__((nosvm))
ClosedPublic

Authored by yaxunl on Mar 3 2016, 12:00 PM.

Details

Summary

OpenCL 2.0 supports attribute((nosvm)) but it is not widely used and removed from OpenCL 2.1.

This change allows Clang to accept attribute((nosvm)) for OpenCL but ignores it.

Differential Revision: http://reviews.llvm.org/D17861

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl updated this revision to Diff 49763.Mar 3 2016, 12:00 PM
yaxunl retitled this revision from to [OpenCL] Accept __attribute__((nosvm)).
yaxunl updated this object.
yaxunl added a reviewer: Anastasia.
yaxunl set the repository for this revision to rL LLVM.
Anastasia added inline comments.Mar 7 2016, 10:07 AM
test/SemaOpenCL/nosvm.cl
9 ↗(On Diff #49763)

It seems like it makes nosvm available for C and earlier standards of OpenCL too?

Doesn't feel right. :(

yaxunl added inline comments.Mar 7 2016, 11:08 AM
test/SemaOpenCL/nosvm.cl
9 ↗(On Diff #49763)

Before my change, the original behavior of Clang is to emits a warning for any unknown GNU attribute, e.g.
unknown 'nosvm' attribute is ignored.

Should I just emit the same warning if the language is not OpenCL or if the language is OpenCL but the version is not 2.0? Or I should emit an error if the language is OpenCL but the version is not 2.0?

yaxunl added inline comments.Mar 7 2016, 11:56 AM
test/SemaOpenCL/nosvm.cl
9 ↗(On Diff #49763)

Also, `'nosvm' attribute ignored` is the default warning msg used in tablegen when LangOpt<"OpenCL"> is used. The following code is generated by tablegen:

static bool checkOpenCLLangOpts(Sema &S, const AttributeList &Attr) {
  if (S.LangOpts.OpenCL)
    return true;

  S.Diag(Attr.getLoc(), diag::warn_attribute_ignored) << Attr.getName();
  return false;
}

Basically, if an attrib is defined for a specific language by using LangOpt<> in Attr.td, when this attr is used in other languages, the original behavior of Clang is to emit a warning like:

'nosvm' attribute ignored

Do we want to fix this? Or simply don't use LangOpt<> for this attribute.

Anastasia added inline comments.Mar 8 2016, 10:45 AM
test/SemaOpenCL/nosvm.cl
9 ↗(On Diff #49763)

I am a bit confused now. So what diagnostic would Clang give if we don't use LanOpt<>? Is it the same as if with LangOpt<>? Is there anything we can do about CL version as well?

I don't feel like we should modify default Clang behavior wrt attributes a lot. Let's try to find the best way to use what we already have.

yaxunl added inline comments.Mar 8 2016, 11:09 AM
test/SemaOpenCL/nosvm.cl
9 ↗(On Diff #49763)

For non-OpenCL programs,

If we don't use LangOpt, Clang will emit warning

unknown 'nosvm' attribute ignored

If we use LangOpt, Clang will emit warning

'nosvm' attribute ignored

If we don't want to change Clang default behavior but also don't like the second default warning, we should not use LangOpt.

I can emit a warning for OpenCL program which version is not 2.0:

'nosvm' attribute is valid only for OpenCL 2.0

and emit a warning for non-OpenCL programs

unknown 'nosvm' attribute is ignored
Anastasia edited edge metadata.Mar 9 2016, 4:57 AM

Please, add cfe-commit to Subscribers!

yaxunl edited edge metadata.Mar 9 2016, 7:29 AM
yaxunl added a subscriber: cfe-commits.
Anastasia added inline comments.Mar 9 2016, 10:28 AM
test/SemaOpenCL/nosvm.cl
9 ↗(On Diff #49763)

Yes, seems like it's best not to use LangOpt actually.

So if the change is not large I would prefer the second option you proposed!

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
include/clang/Basic/Attr.td
701 ↗(On Diff #49763)

Since the attribute is ignored by clang, you should inherit from IgnoredAttr.

yaxunl marked 7 inline comments as done.Mar 9 2016, 10:47 AM
yaxunl added inline comments.
include/clang/Basic/Attr.td
701 ↗(On Diff #49763)

I tried that from beginning. If I inherit from IgnoredAttr, it seems to be ignored by the parser and won't reach the sema check part, and I cannot emit error msg based on OpenCL version.

aaron.ballman added inline comments.Mar 9 2016, 10:57 AM
include/clang/Basic/Attr.td
701 ↗(On Diff #49763)

Ah. so it isn't *totally* ignored. Okay, in that case, you should set ASTNode = 0 and SemaHandler = 0.

yaxunl marked an inline comment as done.Mar 22 2016, 10:01 AM
yaxunl added inline comments.
include/clang/Basic/Attr.td
701 ↗(On Diff #49763)

If I set ASTNode = 0 and SemaHandler = 0, tablegen will not generate an id AT_OpenCLNoSVM to identify this attribute, and I cannot diagnose its invalid usage in SemaDeclAttr.cpp due to that. Then it seems the only viable place to diagnose its invalid usage is in Parser::ParseGNUAttributes. However currently Parser::ParseGNUAttributes only does generic processing of GNU attributes and does not diagnose specific attribute. Since there is no id for NoSVM attribute, I have to diagnose based on its name string.

Do we really want to do this?

aaron.ballman added inline comments.Mar 28 2016, 6:29 AM
include/clang/Basic/Attr.td
701 ↗(On Diff #49763)

Oh! I forgot that setting SemaHandler to 0 means that the AttributeList enumeration won't get an entry for the attribute, so we probably diagnose it as being unknown rather than getting to the language option checking. Ugh, sorry for the noise, you will want SemaHandler = 1 still (which it is by default, so no need to explicitly specify that), but ASTNode = 0 since we don't generate any AST information for the attribute.

yaxunl updated this revision to Diff 51821.Mar 28 2016, 12:24 PM
yaxunl removed rL LLVM as the repository for this revision.
yaxunl marked 4 inline comments as done.

Revised by Aaron's and Anastasia's comments.

Changed error/warning message for invalid usage of nosvm attribute.

aaron.ballman requested changes to this revision.Mar 28 2016, 1:45 PM
aaron.ballman edited edge metadata.
aaron.ballman added inline comments.
lib/Sema/SemaDeclAttr.cpp
5210 ↗(On Diff #51821)

Please use the language option feature in tabelgen as in your previous patch instead of performing this work manually. We want use of the attribute to provide a different diagnostic in this case because the attribute *isn't* unknown, it simply doesn't apply under the compilation target (and hence is ignored).

5218 ↗(On Diff #51821)

This should have an else clause telling the user that the attribute is being ignored; silently accepting it and then not performing the semantics of the attribute will be confusing behavior for most users. It might not be a bad idea to also have the diagnostic tell the user that the attribute has been deprecated and is removed in OpenCL 2.1.

This revision now requires changes to proceed.Mar 28 2016, 1:45 PM
yaxunl updated this revision to Diff 51967.Mar 29 2016, 1:11 PM
yaxunl edited edge metadata.
yaxunl marked 2 inline comments as done.

Revised as Aaron suggested.

aaron.ballman accepted this revision.Mar 29 2016, 1:32 PM
aaron.ballman edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Mar 29 2016, 1:32 PM
Anastasia accepted this revision.Mar 30 2016, 4:00 AM
Anastasia edited edge metadata.

LGTM, except for one tiny comment that can be addressed before committing.

test/SemaOpenCL/nosvm.cl
11 ↗(On Diff #51967)

Separate with an empty line here.

yaxunl updated this revision to Diff 52077.Mar 30 2016, 9:36 AM
yaxunl edited edge metadata.
yaxunl marked an inline comment as done.

Insert an empty line to the test for clarity.

yaxunl updated this object.Mar 30 2016, 9:40 AM
This revision was automatically updated to reflect the committed changes.