[PATCH] #pragma vectorize
Alexey Bataev
a.bataev at hotmail.com
Mon May 5 20:22:24 PDT 2014
Hello Tyler,
Here are my comments:
1. include/clang/AST/Attr.h
#include "clang/AST/AttrIterator.h"
#include "clang/AST/Decl.h"
#include "clang/AST/Type.h"
+#include "clang/AST/Expr.h"
#include "clang/Basic/AttrKinds.h"
#include "clang/Basic/LLVM.h"
#include "clang/Basic/SourceLocation.h"
The header files must be sorted lexicographically by the full path.
2. lib/AST/StmtPrinter.cpp
+ if (isa<LoopHintAttr>(*it)) {
+ const LoopHintAttr *LHA = cast<LoopHintAttr>(*it);
+ Indent();
+ LHA->printPragma(OS, Policy);
+ } else {
It would be better to rewrite it the follwing way:
+ if (const LoopHintAttr *LHA = dyn_cast<LoopHintAttr>(*it)) {
+ Indent();
+ LHA->printPragma(OS, Policy);
+ } else {
Besides, I'm not sure that there is a need to call Indent() for pragma
printing. Pragmas must be unindented.
3. lib/CodeGen/CGStmt.cpp
#include "clang/AST/StmtVisitor.h"
#include "clang/Basic/PrettyStackTrace.h"
#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/LoopHint.h"
The header files must be sorted lexicographically by the full path.
+ llvm::IntegerType *IntTy = llvm::Type::getInt32Ty(Context);
No need to redefine int type, CodeGenFunction already has Int32Ty (for
32-bit int) and IntTy (for int representation)
+ 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();
+ continue;
+ }
+ }
+
+ llvm::Value *Value;
+ llvm::MDString *Name;
+ LoopHintAttr::Spelling S = LH->getSemanticSpelling();
+
+ if (S == LoopHintAttr::Keyword_vectorize) {
+ // Do not add hint if it is incompatible with prior hints.
+ if (!LoopHintAttr::isCompatible(VectorizeState | Type)) {
+ CGM.getDiags().Report(LH->getRange().getEnd(),
+ diag::warn_pragma_loop_incompatible) <<
+ LoopHintAttr::getName(VectorizeState) <<
+ LoopHintAttr::getName(Type) <<
+ LH->getSpelling();
+ continue;
+ }
+ } else if (S == LoopHintAttr::Keyword_interleave) {
+ // Do not add hint if it is incompatible with prior hints.
+ if (!LoopHintAttr::isCompatible(InterleaveState | Type)) {
+ CGM.getDiags().Report(LH->getRange().getEnd(),
+ diag::warn_pragma_loop_incompatible) <<
+ LoopHintAttr::getName(InterleaveState) <<
+ LoopHintAttr::getName(Type) <<
+ LH->getSpelling();
+ continue;
+ }
I think it should be verified in Sema, not in CodeGen
+ Value = llvm::ConstantInt::get(BoolTy, true);
I think it would be better to use Builder.getTrue().
4. lib/Parse/ParsePragma.cpp
#include "clang/Parse/ParseDiagnostic.h"
#include "clang/Parse/Parser.h"
#include "clang/Sema/Scope.h"
+#include "clang/Basic/LoopHint.h"
#include "llvm/ADT/StringSwitch.h"
The header files must be sorted lexicographically by the full path.
BalancedDelimiterTracker is not used for parsing.
5. lib/Parse/ParseStmt.cpp
#include "clang/Basic/PrettyStackTrace.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/LoopHint.h"
+#include "clang/Basic/Attributes.h"
#include "clang/Sema/DeclSpec.h"
#include "clang/Sema/PrettyDeclStackTrace.h"
#include "clang/Sema/Scope.h"
The header files must be sorted lexicographically by the full path.
+ case tok::annot_pragma_loop_hint:
+ ProhibitAttributes(Attrs);
+ return ParsePragmaLoopHint(Stmts, OnlyStatement, TrailingElseLoc,
Attrs);
Why attributes are prohibited?
+ if (Tok.is(tok::kw___attribute) &&
+ (NextTok.is(tok::kw_do) ||
+ NextTok.is(tok::kw_for) ||
+ NextTok.is(tok::kw_while)) ) {
+ // Get the loop's attributes.
+ MaybeParseCXX11Attributes(Attrs, 0, /*MightBeObjCMessageSend*/
true);
+ }
I don't think that this correct to handle attributed statements. C++11
does not use __attribute as a key word for attributes, but '[['
sequence. I think it would be better just to call
MaybeParseCXX11Attributes(...) without any preconditions. Besides,
AttributedStmt will be never created, because you don't call
Sema::ProcessStmtAttributes(...) after all.
I think you need to add tests for attributes.
6. lib/Sema/SemaStmtAttr.cpp
+ if (St->getStmtClass() != Stmt::DoStmtClass &&
+ St->getStmtClass() != Stmt::ForStmtClass &&
+ St->getStmtClass() != Stmt::CXXForRangeStmtClass &&
+ St->getStmtClass() != Stmt::WhileStmtClass) {
+ S.Diag(St->getLocStart(), diag::warn_pragma_loop_precedes_nonloop);
+ return 0;
+ }
AttributedStmts are not allowed?
7. test/PCH/pragma-loop.cpp
I think all allowed variants of enable|disable|value must be included
in this test to verify serialization/deserialization/ast-printing.
Best regards,
Alexey Bataev
=============
Software Engineer
Intel Compiler Team
Intel Corp.
5 Май 2014 г. 22:41:49, Tyler Nowicki писал:
> Hello Everyone,
>
> Thank you for all the input again. Its been great and I’m learning a
> lot. I’ve modified the patch to handle ast-print and
> serialization/deserialization. Tests have been added for both of
> these. Several other changes have been made to improve the patch.
> Please review the patch and let me know if you have any concerns.
>
> Non-type template parameters are currently not supported and will be
> added later.
>
> Thanks again,
>
> Tyler
>
>
>
>
> On May 1, 2014, at 5:00 PM, Richard Smith <richard at metafoo.co.uk
> <mailto:richard at metafoo.co.uk>> wrote:
>
>> On Thu, May 1, 2014 at 4:40 PM, Tyler Nowicki <tnowicki at apple.com
>> <mailto:tnowicki at apple.com>> wrote:
>>
>> Hi Doug,
>>
>> To evaluate the Expr in the CodeGen would you
>> use ConstantFoldsToSimpleInteger(..) to do that and get the
>> integer value?
>>
>>
>> You should presumably be checking that the expression is an integral
>> constant expression during template instantiation, and not waiting
>> until CodeGen. For that, use Sema::VerifyIntegerConstantExpression.
>>
>> Tyler
>>
>>
>> On May 1, 2014, at 10:19 AM, Douglas Gregor <dgregor at apple.com
>> <mailto:dgregor at apple.com>> wrote:
>>
>>> … I somehow managed to review an old patch. Tyler and I spoke
>>> off-line and I’m looking forward to the next revision!
>>>
>>> - Doug
>>>
>>> On May 1, 2014, at 8:12 AM, Douglas Gregor <dgregor at apple.com
>>> <mailto:dgregor at apple.com>> wrote:
>>>
>>>> Hi Tyler,
>>>>
>>>> On Apr 29, 2014, at 3:51 PM, Tyler Nowicki <tnowicki at apple.com
>>>> <mailto:tnowicki at apple.com>> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> I’ve updated the patch with the FIXME. I’ve also added a
>>>>> separate test for the contradictory pragmas.
>>>>>
>>>>> @Alexander: Since BalancedDelimiterTracker does not have any
>>>>> benefits and just adds unnecessary complexity I opted not to
>>>>> use it.
>>>>>
>>>>> Thanks everyone for your feedback. Please review the updated
>>>>> patch.
>>>>
>>>>
>>>> +/// LoopVectorizeHints - This provides the interface
>>>> +/// for specifying and retrieving vectorization hints
>>>> +///
>>>> +class LoopVectorizeHints {
>>>> + SmallVector<VectorizeHint, 1> CondBrVectorizeHints;
>>>> AST nodes are never destroyed, so any memory they refer to must
>>>> be allocated through the ASTContext. SmallVectors or other
>>>> memory-holding data structures in AST nodes will leak. Please
>>>> use either an ArrayRef that refers to ASTContext-allocated
>>>> memory or (if you must resize after initializing constructing
>>>> the AST node) ASTVector for this. However, hold that thought…
>>>> better idea coming below.
>>>>
>>>> + /// Beginning of list of vectorization hints
>>>> + SmallVectorImpl<VectorizeHint>::const_iterator
>>>> beginCondBrVecHints() const {
>>>> + return CondBrVectorizeHints.begin();
>>>> + }
>>>> +
>>>> + /// Terminator of vectorization hints list
>>>> + SmallVectorImpl<VectorizeHint>::const_iterator
>>>> endCondBrVecHints() const {
>>>> + return CondBrVectorizeHints.end();
>>>> + }
>>>>
>>>> We tend to use STL-ish “_begin” and “_end” when naming the
>>>> functions that get iterators. Do you want to provide just the
>>>> for-range-loop-friendly
>>>>
>>>> ArrayRef<VectorizeHint> getCondBrVecHints() const { .. }
>>>>
>>>> signature instead?
>>>>
>>>> /// WhileStmt - This represents a 'while' stmt.
>>>> ///
>>>> -class WhileStmt : public Stmt {
>>>> +class WhileStmt : public Stmt, public LoopVectorizeHints {
>>>> enum { VAR, COND, BODY, END_EXPR };
>>>>
>>>> I see that WhileStmt, DoWhileStmt, and ForStmt are covered. I
>>>> assume that CXXForRangeStmt and ObjCForCollectionStmt should
>>>> also get this behavior, which implies that we should just bite
>>>> the bullet and add an abstract class LoopStmt from which these
>>>> all inherit and where this functionality lives.
>>>>
>>>> We shouldn’t bloat the size of every WhileStmt for the
>>>> (extremely rare) case where the loop has vectorization hints.
>>>> Here’s an alternative approach: add a bit down in Stmt (e.g.,
>>>> in a new LoopStmtBitFields) that indicates the presence of loop
>>>> vectorization hints. Then, add to ASTContext a DenseMap from
>>>> LoopStmt*’s with this bit set to the corresponding
>>>> LoopVectorizeHints structure, i.e.,
>>>>
>>>> llvm::DenseMap<LoopStmt *, LoopVectorizeHints>
>>>> AllLoopVectorizeHints;
>>>>
>>>> The ASTContext *does* get destroyed, so memory will get cleaned
>>>> up even when you’re using SmallVector in LoopVectorizeHints.
>>>>
>>>> The accessors to get at the LoopVectorizeHints element for a
>>>> LoopStmt should still be on the LoopStmt node, so the API is
>>>> the same, but it costs nothing in the common case (the bit
>>>> you’ll be stealing is just padding now). This is how we handle
>>>> declaration attributes, among other things. It’s a good pattern.
>>>>
>>>> +enum VectorizeHintKind {
>>>> + VH_UNKNOWN,
>>>> + VH_ENABLE,
>>>> + VH_DISABLE,
>>>> + VH_WIDTH,
>>>> + VH_UNROLL
>>>> +};
>>>>
>>>> Doxygen comment, please! Also, the names should follow LLVM
>>>> coding style:
>>>>
>>>> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
>>>>
>>>> i.e., VH_Unknown, VH_Enable, VH_Disable.
>>>>
>>>> +/// \brief Vectorization hint specified by a pragma vectorize
>>>> +/// and used by codegen to attach metadata to the IR
>>>> +struct VectorizeHint {
>>>> + VectorizeHintKind Kind;
>>>> + uint64_t Value;
>>>> +};
>>>>
>>>> Alexey is right that this will need to change to support
>>>> non-type template arguments. You’ll likely end up with an Expr*
>>>> here instead of Value, and will use the constant evaluator in
>>>> CodeGen to get the value. I’m okay with this coming in a
>>>> follow-up patch.
>>>>
>>>> + switch(I->Kind) {
>>>> + case VH_ENABLE:
>>>> + Name = llvm::MDString::get(Context,
>>>> "llvm.vectorizer.enable");
>>>> + Value = llvm::ConstantInt::get(BoolTy, true);
>>>> + break;
>>>> + case VH_DISABLE:
>>>> + Name = llvm::MDString::get(Context,
>>>> "llvm.vectorizer.enable");
>>>> + Value = llvm::ConstantInt::get(BoolTy, false);
>>>> + break;
>>>> + case VH_WIDTH:
>>>> + Name = llvm::MDString::get(Context,
>>>> "llvm.vectorizer.width");
>>>> + Value = llvm::ConstantInt::get(IntTy, I->Value);
>>>> + break;
>>>> + case VH_UNROLL:
>>>> + Name = llvm::MDString::get(Context,
>>>> "llvm.vectorizer.unroll");
>>>> + Value = llvm::ConstantInt::get(IntTy, I->Value);
>>>> + break;
>>>> + default:
>>>> + continue;
>>>> + }
>>>>
>>>> Please replace the “default:” with “case VH_UNKNOWN:”. We like
>>>> to fully cover our enums in switches, so that when someone adds
>>>> a new VH_* constant, compiler warnings direct them to
>>>> everything that needs to be updated.
>>>>
>>>> + // Verify that this is one of the whitelisted vectorize hints
>>>> + IdentifierInfo *II = Tok.getIdentifierInfo();
>>>> + VectorizeHintKind Kind =
>>>> + llvm::StringSwitch<VectorizeHintKind>(II->getName())
>>>> + .Case("enable", VH_ENABLE)
>>>> + .Case("disable", VH_DISABLE)
>>>> + .Case("width", VH_WIDTH)
>>>> + .Case("unroll", VH_UNROLL)
>>>> + .Default(VH_UNKNOWN);
>>>>
>>>> Since we’re not actually creating VH_UNKNOWNs in the AST,
>>>> there’s no reason to have VH_UNKNOWN. Why not make this
>>>> StringSwitch produce an Optional<VectorizeHintKind>, so
>>>> VK_UNKNOWN can go away?
>>>>
>>>> + // Verify it is a loop
>>>> + if (isa<WhileStmt>(Loop)) {
>>>> + WhileStmt *While = cast<WhileStmt>(Loop);
>>>> + While->addCondBrVectorizeHint(Hint);
>>>> + } else if (isa<DoStmt>(Loop)) {
>>>> + DoStmt *Do = cast<DoStmt>(Loop);
>>>> + Do->addCondBrVectorizeHint(Hint);
>>>> + } else if (isa<ForStmt>(Loop)) {
>>>> + ForStmt *For = cast<ForStmt>(Loop);
>>>> + For->addCondBrVectorizeHint(Hint);
>>>> + }
>>>>
>>>> This would be so much easier with a LoopStmt abstract base class.
>>>>
>>>> + if (Tok.isNot(tok::l_paren)) {
>>>> + PP.Diag(Tok.getLocation(), diag::err_expected) <<
>>>> tok::l_paren;
>>>> + return;
>>>> + }
>>>> +
>>>> + PP.Lex(Tok);
>>>> + if (Tok.isNot(tok::numeric_constant) ||
>>>> + !PP.parseSimpleIntegerLiteral(Tok, Hint->Value) ||
>>>> + Hint->Value <= 1) {
>>>> + PP.Diag(Tok.getLocation(), diag::err_expected) <<
>>>> "positive integer";
>>>> + }
>>>> +
>>>> + if (Tok.isNot(tok::r_paren)) {
>>>> + PP.Diag(Tok.getLocation(), diag::err_expected) <<
>>>> tok::r_paren;
>>>> + return;
>>>> + }
>>>>
>>>> I think Alexander is right about BalancedDelimiterTracker.
>>>> Among other things, it gives better recovery on overly-nested
>>>> code and provides better diagnostics and recovery when the ‘)’
>>>> is missing than your hand-coded solution.
>>>>
>>>> As Alexey notes, (de-)serialization and AST printing and AST
>>>> dumping are important to handle. I’m okay with those being
>>>> follow-up patches as well.
>>>>
>>>> - Doug
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
More information about the cfe-commits
mailing list