[PATCH] #pragma vectorize
Tyler Nowicki
tnowicki at apple.com
Wed May 7 18:04:24 PDT 2014
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.
More information about the cfe-commits
mailing list