[PATCH] #pragma vectorize

[Aaron Ballman]

On Wed, May 7, 2014 at 9:04 PM, Tyler Nowicki <tnowicki at apple.com> wrote:
> Hi Aaron,
>
> Thanks for the review. I made many of the changes you suggested and I have some other responses are below. I’ll post a patch shortly.
>
> Tyler
>
>>> #include "clang/AST/AttrIterator.h"
>>> #include "clang/AST/Decl.h"
>>> +#include "clang/AST/Expr.h"
>>
>> Is there a reason this include is required in Attr.h?
>
> Yup, the function printPragma() in clang/Basic/Attr.td needs it to pretty print the pragma value.

I think the entire printPragma function should actually be table
generated, which would then solve the comments about
StmtPrinter::VisitAttributedStmt (since printPretty would do the right
thing).

>
>
>>> +  let Documentation = [Undocumented];
>>
>> I think this should be documented, but I can understand not doing it
>> as part of the attribute documentation. As Richard suggested, if we
>> had a Pragma spelling, that would be beneficial here as well (we could
>> generate a pragma-specific section of the documentation).
>
> This sounds good. I’ll do that in a follow-up commit.
>
> A spelling is needed in order for tablegen to generate the class. Do you suggest modifying
> tablegen to generate code when a PragmaSpelling is specified?

A spelling is not needed for tablegen to generate the class (there are
several attributes which have no spelling and are created implicitly).
But I would suggest adding a new spelling called Pragma (like we
already have Declspec, Keyword, etc). However, that seems a bit more
like a predecessor patch to this one.

>
> I was hoping that by turning the pragmas into attributes I would be able to easily support
> c++11 loop attributes. Something like [[loop vectorize(4)]] for (…) {}. Do you think there is
> a need for a pragma spelling if an equivalent attribute exists?

You would have to make that [[clang::loop_vectorize(4)]] or
[[clang::loop("vectorize", 4)]], or something like that. And yes, that
would require a new spelling (a CXX11 spelling, to be exact). I think
the Pragma spelling would still be useful, but possibly less so for
just your particular use case.

>
>
>>> +#ifndef LLVM_CLANG_BASIC_LOOPHINT_H
>>> +#define LLVM_CLANG_BASIC_LOOPHINT_H
>>> +
>>> +#include "clang/Basic/IdentifierTable.h"
>>> +#include "clang/Basic/SourceLocation.h"
>>> +#include "clang/Sema/AttributeList.h"
>>> +#include "clang/Sema/Ownership.h"
>>
>> This is a layering violation (to include things from Sema into Basic).
>
> Richard suggested I put LoopHint.h into clang/Sema/. Is it a layering violation to do the reverse?

No -- things from Sema can include things from Basic, but not vice
versa. Basic is where we put functionality that is used more
universally, whereas Sema is purely for semantics.

>
>
>>> +    // FIXME: When we get more attributes which have a pragma
>>> +    // form LoopHintAttr should be replaced with a base class
>>> +    // for pretty printing these types of attributes.
>>> +    if (const LoopHintAttr *LHA = dyn_cast<LoopHintAttr>(*it))
>>> +      LHA->printPragma(OS, Policy);
>>
>> I'm not too keen on this approach. Everything should be handled
>> through printPretty, which is generated in ClangAttrEmitter.cpp. This
>> further suggests needing a Pragma spelling for the attribute.
>
> I tried to overload prettyPrint and ran into some problems. I will address it in a follow up patch. But I agree. I didn’t really want to do this either.
>
>
>>> +    const LoopHintAttr *LH = cast<LoopHintAttr>(*I);
>>> +
>>> +    Expr *ValueExpr = LH->getValue();
>>> +    unsigned int Type = LH->getType();
>>> +
>>> +    int ValueInt = 1;
>>> +    if (Type == LoopHintAttr::Value) {
>>> +      llvm::APSInt ValueAPS;
>>> +      if(!ValueExpr || !ValueExpr->EvaluateAsInt(ValueAPS, CGM.getContext()) ||
>>> +         (ValueInt = ValueAPS.getSExtValue()) < 1) {
>>> +        CGM.getDiags().Report(LH->getRange().getEnd(),
>>> +                        diag::warn_pragma_loop_invalid_value) <<
>>> +                       LH->getSpelling();
>>
>> I may be incorrect, but the diagnostics engine should understand how
>> to report the attribute without resorting to calling getSpelling on
>> it. At least, that's the way it works in Sema (and CG should behave
>> the same way if it doesn't already). This comment applies to all
>> diagnostic reporting in the patch, not just this one.
>
> Doesn’t seem to, passing the object fails to compile and passing the pointer causes a seg fault.

Hmm, that seems a bit odd to me -- Attr.h should already handle this
(at the bottom). Definitely worth exploring.

>
>
>>> +#include "clang/Basic/LoopHint.h"
>>> +#include "clang/Lex/Lexer.h"
>>> +#include "clang/Parse/ParseDiagnostic.h"
>>
>> Why are parser diagnostics required for sema? That seems wrong.
>
> The diagnostic error message ‘err_pragma_loop_precedes_nonloop’ is used here. It is cleaner to verify that the substmt is a loop in the attribute handler than it is in pragma loop hint parser.

Sorry, I wasn't very clear -- generally speaking, parser diagnostics
are fired from Parse files, and sema diagnostics are fired from Sema
files. So it doesn't sit well with me that a Sema file is firing
parser diagnostics.

~Aaron