[PATCH] #pragma vectorize

[Tyler Nowicki]

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.


>> +  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?

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?


>> +#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?


>> +    // 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.


>> +#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.