[OpenACC] Private Clause on Compute Constructs #90521
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The private clause is the first that takes a 'var-list', thus this has a lot of additional work to enable the var-list type. A 'var' is a traditional variable reference, subscript, member-expression, or array-section, so checking of these is pretty minor. Note: This ran into some issues with array-sections (aka sub-arrays) that will be fixed in a follow-up patch.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Erich Keane (erichkeane) ChangesThe private clause is the first that takes a 'var-list', thus this has a lot of additional work to enable the var-list type. A 'var' is a traditional variable reference, subscript, member-expression, or array-section, so checking of these is pretty minor. Note: This ran into some issues with array-sections (aka sub-arrays) that will be fixed in a follow-up patch. Patch is 73.67 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/90521.diff 21 Files Affected:
diff --git a/clang/include/clang/AST/OpenACCClause.h b/clang/include/clang/AST/OpenACCClause.h
index 277a351c49fcb8..dafcad4179a37e 100644
--- a/clang/include/clang/AST/OpenACCClause.h
+++ b/clang/include/clang/AST/OpenACCClause.h
@@ -156,51 +156,50 @@ class OpenACCSelfClause : public OpenACCClauseWithCondition {
Expr *ConditionExpr, SourceLocation EndLoc);
};
-/// Represents a clause that has one or more IntExprs. It does not own the
-/// IntExprs, but provides 'children' and other accessors.
-class OpenACCClauseWithIntExprs : public OpenACCClauseWithParams {
- MutableArrayRef<Expr *> IntExprs;
+/// Represents a clause that has one or more expressions associated with it.
+class OpenACCClauseWithExprs : public OpenACCClauseWithParams {
+ MutableArrayRef<Expr *> Exprs;
protected:
- OpenACCClauseWithIntExprs(OpenACCClauseKind K, SourceLocation BeginLoc,
- SourceLocation LParenLoc, SourceLocation EndLoc)
+ OpenACCClauseWithExprs(OpenACCClauseKind K, SourceLocation BeginLoc,
+ SourceLocation LParenLoc, SourceLocation EndLoc)
: OpenACCClauseWithParams(K, BeginLoc, LParenLoc, EndLoc) {}
/// Used only for initialization, the leaf class can initialize this to
/// trailing storage.
- void setIntExprs(MutableArrayRef<Expr *> NewIntExprs) {
- assert(IntExprs.empty() && "Cannot change IntExprs list");
- IntExprs = NewIntExprs;
+ void setExprs(MutableArrayRef<Expr *> NewExprs) {
+ assert(Exprs.empty() && "Cannot change Exprs list");
+ Exprs = NewExprs;
}
- /// Gets the entire list of integer expressions, but leave it to the
+ /// Gets the entire list of expressions, but leave it to the
/// individual clauses to expose this how they'd like.
- llvm::ArrayRef<Expr *> getIntExprs() const { return IntExprs; }
+ llvm::ArrayRef<Expr *> getExprs() const { return Exprs; }
public:
child_range children() {
- return child_range(reinterpret_cast<Stmt **>(IntExprs.begin()),
- reinterpret_cast<Stmt **>(IntExprs.end()));
+ return child_range(reinterpret_cast<Stmt **>(Exprs.begin()),
+ reinterpret_cast<Stmt **>(Exprs.end()));
}
const_child_range children() const {
child_range Children =
- const_cast<OpenACCClauseWithIntExprs *>(this)->children();
+ const_cast<OpenACCClauseWithExprs *>(this)->children();
return const_child_range(Children.begin(), Children.end());
}
};
class OpenACCNumGangsClause final
- : public OpenACCClauseWithIntExprs,
+ : public OpenACCClauseWithExprs,
public llvm::TrailingObjects<OpenACCNumGangsClause, Expr *> {
OpenACCNumGangsClause(SourceLocation BeginLoc, SourceLocation LParenLoc,
ArrayRef<Expr *> IntExprs, SourceLocation EndLoc)
- : OpenACCClauseWithIntExprs(OpenACCClauseKind::NumGangs, BeginLoc,
- LParenLoc, EndLoc) {
+ : OpenACCClauseWithExprs(OpenACCClauseKind::NumGangs, BeginLoc, LParenLoc,
+ EndLoc) {
std::uninitialized_copy(IntExprs.begin(), IntExprs.end(),
getTrailingObjects<Expr *>());
- setIntExprs(MutableArrayRef(getTrailingObjects<Expr *>(), IntExprs.size()));
+ setExprs(MutableArrayRef(getTrailingObjects<Expr *>(), IntExprs.size()));
}
public:
@@ -209,35 +208,35 @@ class OpenACCNumGangsClause final
ArrayRef<Expr *> IntExprs, SourceLocation EndLoc);
llvm::ArrayRef<Expr *> getIntExprs() {
- return OpenACCClauseWithIntExprs::getIntExprs();
+ return OpenACCClauseWithExprs::getExprs();
}
llvm::ArrayRef<Expr *> getIntExprs() const {
- return OpenACCClauseWithIntExprs::getIntExprs();
+ return OpenACCClauseWithExprs::getExprs();
}
};
/// Represents one of a handful of clauses that have a single integer
/// expression.
-class OpenACCClauseWithSingleIntExpr : public OpenACCClauseWithIntExprs {
+class OpenACCClauseWithSingleIntExpr : public OpenACCClauseWithExprs {
Expr *IntExpr;
protected:
OpenACCClauseWithSingleIntExpr(OpenACCClauseKind K, SourceLocation BeginLoc,
SourceLocation LParenLoc, Expr *IntExpr,
SourceLocation EndLoc)
- : OpenACCClauseWithIntExprs(K, BeginLoc, LParenLoc, EndLoc),
+ : OpenACCClauseWithExprs(K, BeginLoc, LParenLoc, EndLoc),
IntExpr(IntExpr) {
- setIntExprs(MutableArrayRef<Expr *>{&this->IntExpr, 1});
+ setExprs(MutableArrayRef<Expr *>{&this->IntExpr, 1});
}
public:
- bool hasIntExpr() const { return !getIntExprs().empty(); }
+ bool hasIntExpr() const { return !getExprs().empty(); }
const Expr *getIntExpr() const {
- return hasIntExpr() ? getIntExprs()[0] : nullptr;
+ return hasIntExpr() ? getExprs()[0] : nullptr;
}
- Expr *getIntExpr() { return hasIntExpr() ? getIntExprs()[0] : nullptr; };
+ Expr *getIntExpr() { return hasIntExpr() ? getExprs()[0] : nullptr; };
};
class OpenACCNumWorkersClause : public OpenACCClauseWithSingleIntExpr {
@@ -261,6 +260,40 @@ class OpenACCVectorLengthClause : public OpenACCClauseWithSingleIntExpr {
Expr *IntExpr, SourceLocation EndLoc);
};
+/// Represents a clause with one or more 'var' objects, represented as an expr,
+/// as its arguments. Var-list is expected to be stored in trailing storage.
+/// For now, we're just storing the original expression in its entirety, unlike
+/// OMP which has to do a bunch of work to create a private.
+class OpenACCClauseWithVarList : public OpenACCClauseWithExprs {
+protected:
+ OpenACCClauseWithVarList(OpenACCClauseKind K, SourceLocation BeginLoc,
+ SourceLocation LParenLoc, SourceLocation EndLoc)
+ : OpenACCClauseWithExprs(K, BeginLoc, LParenLoc, EndLoc) {}
+
+public:
+ ArrayRef<Expr *> getVarList() { return getExprs(); }
+ ArrayRef<Expr *> getVarList() const { return getExprs(); }
+};
+
+class OpenACCPrivateClause final
+ : public OpenACCClauseWithVarList,
+ public llvm::TrailingObjects<OpenACCPrivateClause, Expr *> {
+
+ OpenACCPrivateClause(SourceLocation BeginLoc, SourceLocation LParenLoc,
+ ArrayRef<Expr *> VarList, SourceLocation EndLoc)
+ : OpenACCClauseWithVarList(OpenACCClauseKind::Private, BeginLoc,
+ LParenLoc, EndLoc) {
+ std::uninitialized_copy(VarList.begin(), VarList.end(),
+ getTrailingObjects<Expr *>());
+ setExprs(MutableArrayRef(getTrailingObjects<Expr *>(), VarList.size()));
+ }
+
+public:
+ static OpenACCPrivateClause *
+ Create(const ASTContext &C, SourceLocation BeginLoc, SourceLocation LParenLoc,
+ ArrayRef<Expr *> VarList, SourceLocation EndLoc);
+};
+
template <class Impl> class OpenACCClauseVisitor {
Impl &getDerived() { return static_cast<Impl &>(*this); }
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f72d5c252b863e..b58ef1d451a3ae 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12305,4 +12305,7 @@ def err_acc_num_gangs_num_args
"OpenACC 'num_gangs' "
"%select{|clause: '%1' directive expects maximum of %2, %3 were "
"provided}0">;
+def err_acc_not_a_var_ref
+ : Error<"OpenACC variable is not a valid variable name, sub-array, array "
+ "element, or composite variable member">;
} // end of sema component.
diff --git a/clang/include/clang/Basic/OpenACCClauses.def b/clang/include/clang/Basic/OpenACCClauses.def
index dd5792e7ca8c39..6c3c2db66ef0cf 100644
--- a/clang/include/clang/Basic/OpenACCClauses.def
+++ b/clang/include/clang/Basic/OpenACCClauses.def
@@ -20,6 +20,7 @@ VISIT_CLAUSE(If)
VISIT_CLAUSE(Self)
VISIT_CLAUSE(NumGangs)
VISIT_CLAUSE(NumWorkers)
+VISIT_CLAUSE(Private)
VISIT_CLAUSE(VectorLength)
#undef VISIT_CLAUSE
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index fb117bf04087ee..ae82ebd0586f72 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -3645,11 +3645,12 @@ class Parser : public CodeCompletionHandler {
ExprResult ParseOpenACCIDExpression();
/// Parses the variable list for the `cache` construct.
void ParseOpenACCCacheVarList();
+
+ using OpenACCVarParseResult = std::pair<ExprResult, OpenACCParseCanContinue>;
/// Parses a single variable in a variable list for OpenACC.
- bool ParseOpenACCVar();
- /// Parses the variable list for the variety of clauses that take a var-list,
- /// including the optional Special Token listed for some,based on clause type.
- bool ParseOpenACCClauseVarList(OpenACCClauseKind Kind);
+ OpenACCVarParseResult ParseOpenACCVar();
+ /// Parses the variable list for the variety of places that take a var-list.
+ llvm::SmallVector<Expr *> ParseOpenACCVarList();
/// Parses any parameters for an OpenACC Clause, including required/optional
/// parens.
OpenACCClauseParseResult
diff --git a/clang/include/clang/Sema/SemaOpenACC.h b/clang/include/clang/Sema/SemaOpenACC.h
index da19503c2902fd..9c4ca40e34c5c4 100644
--- a/clang/include/clang/Sema/SemaOpenACC.h
+++ b/clang/include/clang/Sema/SemaOpenACC.h
@@ -48,8 +48,12 @@ class SemaOpenACC : public SemaBase {
SmallVector<Expr *> IntExprs;
};
+ struct VarListDetails {
+ SmallVector<Expr *> VarList;
+ };
+
std::variant<std::monostate, DefaultDetails, ConditionDetails,
- IntExprDetails>
+ IntExprDetails, VarListDetails>
Details = std::monostate{};
public:
@@ -112,6 +116,18 @@ class SemaOpenACC : public SemaBase {
return const_cast<OpenACCParsedClause *>(this)->getIntExprs();
}
+ // Non-const version that permits modifying of the VarList for the purposes
+ // of Sema enforcement.
+ SmallVector<Expr *> &getVarList() {
+ assert(ClauseKind == OpenACCClauseKind::Private &&
+ "Parsed clause kind does not have a var-list");
+ return std::get<VarListDetails>(Details).VarList;
+ }
+
+ ArrayRef<Expr *> getVarList() const {
+ return const_cast<OpenACCParsedClause *>(this)->getVarList();
+ }
+
void setLParenLoc(SourceLocation EndLoc) { LParenLoc = EndLoc; }
void setEndLoc(SourceLocation EndLoc) { ClauseRange.setEnd(EndLoc); }
@@ -147,7 +163,19 @@ class SemaOpenACC : public SemaBase {
ClauseKind == OpenACCClauseKind::NumWorkers ||
ClauseKind == OpenACCClauseKind::VectorLength) &&
"Parsed clause kind does not have a int exprs");
- Details = IntExprDetails{IntExprs};
+ Details = IntExprDetails{std::move(IntExprs)};
+ }
+
+ void setVarListDetails(ArrayRef<Expr *> VarList) {
+ assert(ClauseKind == OpenACCClauseKind::Private &&
+ "Parsed clause kind does not have a var-list");
+ Details = VarListDetails{{VarList.begin(), VarList.end()}};
+ }
+
+ void setVarListDetails(llvm::SmallVector<Expr *> &&VarList) {
+ assert(ClauseKind == OpenACCClauseKind::Private &&
+ "Parsed clause kind does not have a var-list");
+ Details = VarListDetails{std::move(VarList)};
}
};
@@ -194,6 +222,10 @@ class SemaOpenACC : public SemaBase {
ExprResult ActOnIntExpr(OpenACCDirectiveKind DK, OpenACCClauseKind CK,
SourceLocation Loc, Expr *IntExpr);
+ /// Called when encountering a 'var' for OpenACC, ensures it is actually a
+ /// declaration reference to a variable of the correct type.
+ ExprResult ActOnVar(Expr *VarExpr);
+
/// Checks and creates an Array Section used in an OpenACC construct/clause.
ExprResult ActOnArraySectionExpr(Expr *Base, SourceLocation LBLoc,
Expr *LowerBound,
diff --git a/clang/include/clang/Serialization/ASTRecordReader.h b/clang/include/clang/Serialization/ASTRecordReader.h
index 06b80f266a9441..1e11d2d5e42f95 100644
--- a/clang/include/clang/Serialization/ASTRecordReader.h
+++ b/clang/include/clang/Serialization/ASTRecordReader.h
@@ -269,6 +269,9 @@ class ASTRecordReader
/// Read an OpenMP children, advancing Idx.
void readOMPChildren(OMPChildren *Data);
+ /// Read a list of Exprs used for a var-list.
+ llvm::SmallVector<Expr *> readOpenACCVarList();
+
/// Read an OpenACC clause, advancing Idx.
OpenACCClause *readOpenACCClause();
diff --git a/clang/include/clang/Serialization/ASTRecordWriter.h b/clang/include/clang/Serialization/ASTRecordWriter.h
index 1feb8fcbacf772..8b1da49bd4c576 100644
--- a/clang/include/clang/Serialization/ASTRecordWriter.h
+++ b/clang/include/clang/Serialization/ASTRecordWriter.h
@@ -15,6 +15,7 @@
#define LLVM_CLANG_SERIALIZATION_ASTRECORDWRITER_H
#include "clang/AST/AbstractBasicWriter.h"
+#include "clang/AST/OpenACCClause.h"
#include "clang/AST/OpenMPClause.h"
#include "clang/Serialization/ASTWriter.h"
#include "clang/Serialization/SourceLocationEncoding.h"
@@ -293,6 +294,8 @@ class ASTRecordWriter
/// Writes data related to the OpenMP directives.
void writeOMPChildren(OMPChildren *Data);
+ void writeOpenACCVarList(const OpenACCClauseWithVarList *C);
+
/// Writes out a single OpenACC Clause.
void writeOpenACCClause(const OpenACCClause *C);
diff --git a/clang/lib/AST/OpenACCClause.cpp b/clang/lib/AST/OpenACCClause.cpp
index 6cd5b28802187d..208a51c82399cc 100644
--- a/clang/lib/AST/OpenACCClause.cpp
+++ b/clang/lib/AST/OpenACCClause.cpp
@@ -134,6 +134,24 @@ OpenACCNumGangsClause *OpenACCNumGangsClause::Create(const ASTContext &C,
return new (Mem) OpenACCNumGangsClause(BeginLoc, LParenLoc, IntExprs, EndLoc);
}
+OpenACCPrivateClause *OpenACCPrivateClause::Create(const ASTContext &C,
+ SourceLocation BeginLoc,
+ SourceLocation LParenLoc,
+ ArrayRef<Expr *> VarList,
+ SourceLocation EndLoc) {
+ void *Mem = C.Allocate(
+ OpenACCPrivateClause::totalSizeToAlloc<Expr *>(VarList.size()));
+ return new (Mem) OpenACCPrivateClause(BeginLoc, LParenLoc, VarList, EndLoc);
+}
+
+// ValueDecl *getDeclFromExpr(Expr *RefExpr) {
+// //RefExpr = RefExpr->IgnoreParenImpCasts();
+//
+// ////while (isa<ArraySubscriptExpr, ArraySectionExpr>(RefExpr)) {
+// ////}
+// // TODO:
+// }
+
//===----------------------------------------------------------------------===//
// OpenACC clauses printing methods
//===----------------------------------------------------------------------===//
@@ -166,3 +184,9 @@ void OpenACCClausePrinter::VisitVectorLengthClause(
const OpenACCVectorLengthClause &C) {
OS << "vector_length(" << C.getIntExpr() << ")";
}
+
+void OpenACCClausePrinter::VisitPrivateClause(const OpenACCPrivateClause &C) {
+ OS << "private(";
+ llvm::interleaveComma(C.getVarList(), OS);
+ OS << ")";
+}
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index a95f5c6103e24d..973f6f97bae0bf 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -2509,6 +2509,12 @@ void OpenACCClauseProfiler::VisitNumWorkersClause(
Profiler.VisitStmt(Clause.getIntExpr());
}
+void OpenACCClauseProfiler::VisitPrivateClause(
+ const OpenACCPrivateClause &Clause) {
+ for (auto *E : Clause.getVarList())
+ Profiler.VisitStmt(E);
+}
+
void OpenACCClauseProfiler::VisitVectorLengthClause(
const OpenACCVectorLengthClause &Clause) {
assert(Clause.hasIntExpr() &&
diff --git a/clang/lib/AST/TextNodeDumper.cpp b/clang/lib/AST/TextNodeDumper.cpp
index 8f0a9a9b0ed0bc..89f50d6dacfd23 100644
--- a/clang/lib/AST/TextNodeDumper.cpp
+++ b/clang/lib/AST/TextNodeDumper.cpp
@@ -401,6 +401,7 @@ void TextNodeDumper::Visit(const OpenACCClause *C) {
case OpenACCClauseKind::Self:
case OpenACCClauseKind::NumGangs:
case OpenACCClauseKind::NumWorkers:
+ case OpenACCClauseKind::Private:
case OpenACCClauseKind::VectorLength:
// The condition expression will be printed as a part of the 'children',
// but print 'clause' here so it is clear what is happening from the dump.
diff --git a/clang/lib/Parse/ParseOpenACC.cpp b/clang/lib/Parse/ParseOpenACC.cpp
index 29326f5d993a9d..a12ffad699755f 100644
--- a/clang/lib/Parse/ParseOpenACC.cpp
+++ b/clang/lib/Parse/ParseOpenACC.cpp
@@ -86,6 +86,10 @@ OpenACCClauseKind getOpenACCClauseKind(Token Tok) {
if (Tok.is(tok::kw_if))
return OpenACCClauseKind::If;
+ // 'private' is also a keyword, make sure we pare it correctly.
+ if (Tok.is(tok::kw_private))
+ return OpenACCClauseKind::Private;
+
if (!Tok.is(tok::identifier))
return OpenACCClauseKind::Invalid;
@@ -682,28 +686,6 @@ bool Parser::ParseOpenACCIntExprList(OpenACCDirectiveKind DK,
return false;
}
-bool Parser::ParseOpenACCClauseVarList(OpenACCClauseKind Kind) {
- // FIXME: Future clauses will require 'special word' parsing, check for one,
- // then parse it based on whether it is a clause that requires a 'special
- // word'.
- (void)Kind;
-
- // If the var parsing fails, skip until the end of the directive as this is
- // an expression and gets messy if we try to continue otherwise.
- if (ParseOpenACCVar())
- return true;
-
- while (!getCurToken().isOneOf(tok::r_paren, tok::annot_pragma_openacc_end)) {
- ExpectAndConsume(tok::comma);
-
- // If the var parsing fails, skip until the end of the directive as this is
- // an expression and gets messy if we try to continue otherwise.
- if (ParseOpenACCVar())
- return true;
- }
- return false;
-}
-
/// OpenACC 3.3 Section 2.4:
/// The argument to the device_type clause is a comma-separated list of one or
/// more device architecture name identifiers, or an asterisk.
@@ -917,28 +899,19 @@ Parser::OpenACCClauseParseResult Parser::ParseOpenACCClauseParams(
case OpenACCClauseKind::CopyIn:
tryParseAndConsumeSpecialTokenKind(
*this, OpenACCSpecialTokenKind::ReadOnly, ClauseKind);
- if (ParseOpenACCClauseVarList(ClauseKind)) {
- Parens.skipToEnd();
- return OpenACCCanContinue();
- }
+ ParseOpenACCVarList();
break;
case OpenACCClauseKind::Create:
case OpenACCClauseKind::CopyOut:
tryParseAndConsumeSpecialTokenKind(*this, OpenACCSpecialTokenKind::Zero,
ClauseKind);
- if (ParseOpenACCClauseVarList(ClauseKind)) {
- Parens.skipToEnd();
- return OpenACCCanContinue();
- }
+ ParseOpenACCVarList();
break;
case OpenACCClauseKind::Reduction:
// If we're missing a clause-kind (or it is invalid), see if we can parse
// the var-list anyway.
ParseReductionOperator(*this);
- if (ParseOpenACCClauseVarList(ClauseKind)) {
- Parens.skipToEnd();
- return OpenACCCanContinue();
- }
+ ParseOpenACCVarList();
break;
case OpenACCClauseKind::Self:
// The 'self' clause is a var-list instead of a 'condition' in the case of
@@ -958,13 +931,14 @@ Parser::OpenACCClauseParseResult Parser::ParseOpenACCClauseParams(
case OpenACCClauseKind::Link:
case OpenACCClauseKind::NoCreate:
case OpenACCClauseKind::Present:
- case OpenACCClauseKind::Private:
case OpenACCClauseKind::UseDevice:
- if (ParseOpenACCClauseVarList(ClauseKind)) {
- Parens.skipToEnd();
- return OpenACCCanContinue();
- }
+ ParseOpenACCVarList();
+ break;
+ case OpenACCClauseKind::Private: {
+ llvm::SmallVector<Expr *> Vars = ParseOpenACCVarList();
+ ParsedClause.setVarListDetails(std::move(Vars));
break;
+ }
case OpenACCClauseKind::Collapse: {
tryParseAndConsumeSpecialTokenKind(*this, OpenACCSpecialTokenKind::Force,
ClauseKind);
@@ -1227,16 +1201,51 @@ ExprResult Parser::ParseOpenACCBindClauseArgument() {
/// OpenACC 3.3, section 1.6:
/// In this spec, a 'var' (in italics) is one of the following:
-/// - a variable name (a scalar, array, or compisite variable name)
+/// - a variable name (a scalar, array, or composite variable name)
/// - a subarray specification with subscript ranges
/// - an array element
/// - a member of a composite variable
/// - a ...
[truncated]
|
| @@ -112,6 +116,18 @@ class SemaOpenACC : public SemaBase { | |||
| return const_cast<OpenACCParsedClause *>(this)->getIntExprs(); | |||
| } | |||
|
|
|||
| // Non-const version that permits modifying of the VarList for the purposes | |||
| // of Sema enforcement. | |||
| SmallVector<Expr *> &getVarList() { | |||
There was a problem hiding this comment.
Suggested change
| SmallVector<Expr *> &getVarList() { | |
| ArrayRef<Expr *> getVarList() { |
clang/lib/Parse/ParseOpenACC.cpp
Outdated
| break; | ||
| case OpenACCClauseKind::Private: { | ||
| llvm::SmallVector<Expr *> Vars = ParseOpenACCVarList(); | ||
| ParsedClause.setVarListDetails(std::move(Vars)); |
There was a problem hiding this comment.
Why do you need std::move here?
There was a problem hiding this comment.
ParsedClause gets ownership of the vector and it is going out of scope, so it is to save the copy.
Though, I guess doing this whole section as a one liner makes it more clear.
There was a problem hiding this comment.
You mean you want it to be destroyed immediately after setVarListDetails()?
There was a problem hiding this comment.
Right, Vars was immediately going out of scope, so it has no need for its elements anymore, so passing as a 'copy' is not necessary. BUT I've changed it to be an inlined call, so THIS std::move is gone.
Comment on lines
+460
to
+466
| while (isa<ArraySectionExpr, ArraySubscriptExpr>(CurVarExpr)) { | ||
| if (auto *SubScrpt = dyn_cast<ArraySubscriptExpr>(CurVarExpr)) | ||
| CurVarExpr = SubScrpt->getBase()->IgnoreParenImpCasts(); | ||
| else | ||
| CurVarExpr = | ||
| cast<ArraySectionExpr>(CurVarExpr)->getBase()->IgnoreParenImpCasts(); | ||
| } |
There was a problem hiding this comment.
What if the array is casted to a pointer, will this be supported?
There was a problem hiding this comment.
No, a cast is an 'expression', so that makes it not a 'var' reference by my interpretation of the spec. So it is disallowed.
There was a problem hiding this comment.
And what about array deref, which is similar to arr[0]?
There was a problem hiding this comment.
My reading of the standard says 'no', it shouldn't? I wouldn't consider that to be an 'array-element'.
Both of the existing implementations I have access to also reject it: https://godbolt.org/z/Pj98d6c5M
There was a problem hiding this comment.
My reading of the standard says 'no', it shouldn't? I wouldn't consider that to be an 'array-element'.
I don't know, just asking, since it is very similar to what we have in OpenMP :)
There was a problem hiding this comment.
I don't know, just asking, since it is very similar to what we have in OpenMP :)
Yeah, so far I've realized that the OpenACC spec is a very under-specified version of OpenMP in a lot of situations :) I figure I can always loosen these restrictions when/if someone complains.
| if (Res.isUsable()) | ||
| InstantiatedVarList.push_back(Res.get()); | ||
| } | ||
| ParsedClause.setVarListDetails(std::move(InstantiatedVarList)); |
There was a problem hiding this comment.
Why do you need std::move here?
There was a problem hiding this comment.
In this case it is because ParsedClause gets ownership of it, and it isn't used after this, so it is to save the copy.
clang/lib/AST/OpenACCClause.cpp
Outdated
Comment on lines
147
to
154
| // ValueDecl *getDeclFromExpr(Expr *RefExpr) { | ||
| // //RefExpr = RefExpr->IgnoreParenImpCasts(); | ||
| // | ||
| // ////while (isa<ArraySubscriptExpr, ArraySectionExpr>(RefExpr)) { | ||
| // ////} | ||
| // // TODO: | ||
| // } | ||
|
|
There was a problem hiding this comment.
woops! Thanks, good catch!
clang/lib/AST/OpenACCClause.cpp
Outdated
|
|
||
| void OpenACCClausePrinter::VisitPrivateClause(const OpenACCPrivateClause &C) { | ||
| OS << "private("; | ||
| llvm::interleaveComma(C.getVarList(), OS); |
There was a problem hiding this comment.
Do you have a test for this?
There was a problem hiding this comment.
I dont! I THINK this uses 'ast-print', so give me a minute and I'll put one on this review that tests all of the variants here.
There was a problem hiding this comment.
Test added :) A bit of exploration, plus a patch to fix the rest (upstreamed as review after commit), and now up to date here :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The private clause is the first that takes a 'var-list', thus this has a lot of additional work to enable the var-list type. A 'var' is a traditional variable reference, subscript, member-expression, or array-section, so checking of these is pretty minor.
Note: This ran into some issues with array-sections (aka sub-arrays) that will be fixed in a follow-up patch.