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 <[email protected]
<mailto:[email protected]>> wrote:
On Thu, May 1, 2014 at 4:40 PM, Tyler Nowicki <[email protected]
<mailto:[email protected]>> 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 <[email protected]
<mailto:[email protected]>> 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 <[email protected]
<mailto:[email protected]>> wrote:
Hi Tyler,
On Apr 29, 2014, at 3:51 PM, Tyler Nowicki <[email protected]
<mailto:[email protected]>> 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
[email protected] <mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected] <mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
