r239579 - Add support for the the target attribute.

[Eric Christopher]

You get a single email reply for the two of you. :)

Fixed the requests with the following:

dzur:~/sources/llvm/tools/clang> git svn dcommit
Committing to https://llvm.org/svn/llvm-project/cfe/trunk ...
M include/clang/Basic/AttrDocs.td
Committed r241524
M include/clang/Basic/AttrDocs.td
r241524 = 958aa2fcd7ce159211cb76409d97efdbef1d1524
(refs/remotes/origin/master)
M lib/CodeGen/CGCall.cpp
M test/CodeGen/attr-target.c
Committed r241525
M test/CodeGen/attr-target.c
M lib/CodeGen/CGCall.cpp
r241525 = 3b4cd0da85bd32acd27e4459f7e7e9f4658ce289
(refs/remotes/origin/master)
M include/clang/Basic/Attr.td
M lib/CodeGen/CGCall.cpp
Committed r241526
M lib/CodeGen/CGCall.cpp
M include/clang/Basic/Attr.td
r241526 = 7bf902c36d7cbed7343d99c1c6737672d00fd462
(refs/remotes/origin/master)

A few comments:

a) Documentation - it's not good, but you're right, some is better than
nothing. Full documentation of the various backend features sounds like
something that might be handled in a different way, but I'm still thinking
about it. Until then though, some documentation :)

>
> > +      SubjectList<[Function], ErrorDiag,
> "ExpectedFunctionMethodOrClass">;
>
> The diagnostic for this does not match the subject list, and I don't
> understand why. It only applies to functions, but the diagnostic will
> tell the user it can apply to classes and Obj-C methods, which is not
> good.
>

Uh, right. Oops and thanks.


> +    if (FD) {
> > +      if (const TargetAttr *TD = FD->getAttr<TargetAttr>()) {
>
> Better to use const auto here instead of spelling the type twice.
>

Sure. I'm more conservative with my use of auto, but I'm here anyhow.


>
> > +        StringRef FeaturesStr = TD->getFeatures();
> > +        SmallVector<StringRef, 1> AttrFeatures;
> > +        FeaturesStr.split(AttrFeatures, ",");
>
> Does the backend handle spaces and tabs sensibly? eg)
> [[gnu::target("foo, bar,              baz")]]?
>

Nope. Neither did gcc. I've fixed it for us and it handles some weird
whitespace. I don't handle arbitrary whitespace for arch= and tune= but if
you think it's something someone could do I could start regexp matching
them.


> > +static void handleTargetAttr(Sema &S, Decl *D, const AttributeList
> &Attr) {
> > +  // TODO: Validation should use a backend target library that specifies
> > +  // the allowable subtarget features and cpus. We could use something
> like a
> > +  // TargetCodeGenInfo hook here to do validation.
>
> Is there a rough timeline as to when this TODO will be fixed? I'm
> worried about attribute with weird, silent misbehavior if someone has
> a simple typo.
>
>
Well, the backend would complain if it couldn't parse the attribute so I'm
less worried about that - that said, I do plan on adding some verification
of the backend features I know about based on the other work I did for the
__builtin_cpu_supports work.

>
>
> This could be written without the extra declarations, but it's not
> that big of a deal either.
>
>
Enh, I went ahead and left this. It feels more comforting to me :)

Thanks a lot for the review. Very appreciated :)

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150706/afb2bc1b/attachment.html>