================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6961
@@ -6960,1 +6960,3 @@
"variable with local storage in initial value of threadprivate variable">;
+def err_omp_loop_var_dsa : Error<
+ "loop iteration variable may not be %0">;
----------------
Richard Smith wrote:
> What does "dsa" stand for?
Data-sharing Attributes (term from OpenMP, section 2.14.1). I moved this to be
near other _dsa error diagnostic messages.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6963-6975
@@ +6962,15 @@
+ "loop iteration variable may not be %0">;
+def err_omp_loop_not_canonical_init : Error<
+ "initialization of the openmp loop should be assignment to "
+ "(or definition of) the loop variable">;
+def err_omp_loop_not_canonical_init_empty : Error<
+ "initialization of the openmp loop should be non-empty">;
+def err_omp_loop_not_canonical_init_var : Error<
+ "expected variable in the initialization of the openmp loop">;
+def err_omp_loop_not_canonical_init_var_init : Error<
+ "declaration of the loop variable should have initialization">;
+def err_omp_loop_not_canonical_init_var_not_single : Error<
+ "expected exactly one variable in the initialization of the openmp loop">;
+def err_omp_loop_not_canonical_init_many_ctor_params : Error<
+ "too many arguments to the constructor in canonical loop initialization">;
+def err_omp_loop_not_canonical_cond_rel : Error<
----------------
Richard Smith wrote:
> This seems like too many different ways of saying the same thing to me. How
> about something like:
>
> initialization clause of OpenMP for loop must be of the form 'var = init'
> or 'T var = init'
OK
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6976-6979
@@ +6975,6 @@
+ "too many arguments to the constructor in canonical loop initialization">;
+def err_omp_loop_not_canonical_cond_rel : Error<
+ "condition of the openmp loop should be relational operator">;
+def err_omp_loop_not_canonical_cond_rel_var : Error<
+ "relation in the condition of the openmp loop should refer to the loop
variable (%0)">;
+def err_omp_loop_not_canonical_incr_empty : Error<
----------------
Richard Smith wrote:
> How about replacing both of these with:
>
> condition of OpenMP for loop must be a relational comparison ('<', '<=',
> '>', or '>=') of loop variable %0
OK
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6980-6985
@@ +6979,8 @@
+ "relation in the condition of the openmp loop should refer to the loop
variable (%0)">;
+def err_omp_loop_not_canonical_incr_empty : Error<
+ "increment of the openmp loop is empty">;
+def err_omp_loop_not_canonical_incr : Error<
+ "expected increment or decrement of the loop variable by some loop-invariant
step">;
+def err_omp_loop_incr_expected_var : Error<
+ "expected loop variable (%0) in the openmp loop increment">;
+def err_omp_loop_variable_type : Error<
----------------
Richard Smith wrote:
> How about:
>
> increment clause of OpenMP for loop must perform simple addition or
> subtraction on loop variable %0
OK
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6988-6989
@@ +6987,4 @@
+ "variable must be of integer or %select{pointer|random access iterator}0
type">;
+def err_omp_loop_incr_not_integer : Error<
+ "increment expression of for loop must be of an integer type">;
+def err_omp_loop_incr_not_compatible : Error<
----------------
Richard Smith wrote:
> Maybe
>
> increment step of OpenMP for loop is of non-integer type %0
I removed this message - it turns out, this is already checked by
PerformImplicitIntegerConversion.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6991-6992
@@ +6990,4 @@
+def err_omp_loop_incr_not_compatible : Error<
+ "increment expression must cause %0 to %select{decrease|increase}1 "
+ "on each iteration of the loop">;
+def note_omp_loop_cond_requres_compatible_incr : Note<
----------------
Richard Smith wrote:
> Please mention in the diagnostic that the reason for this rule is that this
> is an OpenMP for loop.
OK
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6995-6998
@@ -6961,1 +6994,6 @@
+ "loop step is expected to be %select{negative|positive}0 due to this
condition">;
+def err_omp_loop_cannot_break : Error<
+ "cannot break from the openmp loop">;
+def err_omp_loop_cannot_have_eh : Error<
+ "exception handling constructs are not allowed in a '#pragma omp %0' loop">;
} // end of OpenMP category
----------------
Richard Smith wrote:
> Perhaps replace both of these with
>
> %q0 statement cannot be used in OpenMP for loop
I suggest to change the second message to refer to "OpenMP simd region" - the
first one is related to ('break' stmts in) the loop and the second - to the
loop and all nested loops/other statements.
================
Comment at: include/clang/Sema/Sema.h:7316-7322
@@ -7315,4 +7315,9 @@
SourceLocation StartLoc,
SourceLocation EndLoc);
-
+ /// \brief Called on a for stmt to check and extract its iteration space
+ /// for further processing (such as collapsing).
+ bool CheckOpenMPIterationSpace(OpenMPDirectiveKind DKind, Stmt *S);
+ /// \brief Called on a for stmt to check itself and nested loops (if any).
+ bool CheckOpenMPLoop(OpenMPDirectiveKind DKind, unsigned NestedLoopCount,
+ Stmt *AStmt);
OMPClause *ActOnOpenMPSingleExprClause(OpenMPClauseKind Kind,
----------------
Richard Smith wrote:
> It looks like neither of these needs to be a member of `Sema`: make them
> `static` functions inside SemaOpenMP instead?
Fixed.
================
Comment at: lib/Sema/SemaOpenMP.cpp:819
@@ +818,3 @@
+ /// \brief Reference to Sema.
+ Sema &Actions;
+ /// \brief A location for diagnostics (when there is no some better
location).
----------------
Richard Smith wrote:
> Please call this `SemaRef` or `Owner` or some other common name. The `Parser`
> calls `Sema` `Actions` for reasons that aren't relevant in this case.
Fixed. There are actually several other 'Actions' in SemaOpenMP.cpp unrelated
to my change, I plan to fix them in separate commit.
================
Comment at: lib/Sema/SemaOpenMP.cpp:842
@@ +841,3 @@
+ bool TestIsStrictOp;
+ /// \brief This flag requires to subtract the step instead of adding it.
+ bool SubtractStep;
----------------
Richard Smith wrote:
> This comment is strangely-worded.
Changed to "This flag is true when step is subtracted on each iteration."
================
Comment at: lib/Sema/SemaOpenMP.cpp:852
@@ +851,3 @@
+ /// \brief Check init-expr for canonical loop form and save loop counter
+ /// variable - 'var' and its initialization value - 'lb'.
+ bool CheckInit(Stmt *S);
----------------
Richard Smith wrote:
> Please capitalize these references to other members properly, and refer to
> them as #Var and #LB (that's what makes Doxygen happy, apparently).
OK
================
Comment at: lib/Sema/SemaOpenMP.cpp:866
@@ +865,3 @@
+private:
+ /// \brief Check right-hand side of the increment expression.
+ bool CheckIncRHS(Expr *RHS);
----------------
Richard Smith wrote:
> "Check the right-hand side of an assignment in the increment expression."
OK
================
Comment at: lib/Sema/SemaOpenMP.cpp:882-886
@@ +881,7 @@
+ }
+ QualType VarType = Var->getType()
+ .getNonReferenceType()
+ .getCanonicalType()
+ .getUnqualifiedType();
+ if (VarType->isDependentType())
+ return true;
----------------
Richard Smith wrote:
> This is all redundant. Just use `Var->getType()->isDependentType()`.
OK
================
Comment at: lib/Sema/SemaOpenMP.cpp:888-891
@@ +887,6 @@
+ return true;
+ if (LB &&
+ (LB->isTypeDependent() || LB->isValueDependent() ||
+ LB->isInstantiationDependent() ||
LB->containsUnexpandedParameterPack()))
+ return true;
+ if (UB &&
----------------
Richard Smith wrote:
> This is also redundant. Just `LB && (LB->isValueDependent() ||
> LB->isInstantiationDependent())` would do. (Type-dependence implies
> value-dependence and instantiation-dependence, and an unexpanded pack implies
> instantiation-dependence.)
>
> That said, I think all you actually care about here is value-dependence (that
> is, whether we're able to constant-evaluate the bound).
>
> Likewise for the following two checks.
OK
================
Comment at: lib/Sema/SemaOpenMP.cpp:892-899
@@ +891,10 @@
+ return true;
+ if (UB &&
+ (UB->isTypeDependent() || UB->isValueDependent() ||
+ UB->isInstantiationDependent() ||
UB->containsUnexpandedParameterPack()))
+ return true;
+ if (UB &&
+ (UB->isTypeDependent() || UB->isValueDependent() ||
+ UB->isInstantiationDependent() ||
UB->containsUnexpandedParameterPack()))
+ return true;
+ return false;
----------------
Richard Smith wrote:
> Copy-paste error here. I assume you intended to check `Step`?
Yes, this was a bug.
================
Comment at: lib/Sema/SemaOpenMP.cpp:935-937
@@ +934,5 @@
+ return true;
+ if (!NewStep->isValueDependent() && !NewStep->isTypeDependent() &&
+ !NewStep->isInstantiationDependent() &&
+ !NewStep->containsUnexpandedParameterPack()) {
+
----------------
Richard Smith wrote:
> I think you only care about whether `NewStep` is value-dependent here.
Yes, fixed.
================
Comment at: lib/Sema/SemaOpenMP.cpp:945
@@ +944,3 @@
+ else {
+ Actions.Diag(NewStep->getExprLoc(), diag::err_omp_loop_incr_not_integer);
+ return true;
----------------
Richard Smith wrote:
> You've already issued a diagnostic for this case inside
> `PerformImplicitIntegerConversion`.
OK
================
Comment at: lib/Sema/SemaOpenMP.cpp:961-976
@@ +960,18 @@
+ llvm::APSInt Result;
+ bool IsConstant = NewStep->isIntegerConstantExpr(Result, Actions.Context);
+ bool IsUnsigned = !NewStep->getType()->hasSignedIntegerRepresentation();
+ bool IsConstNeg =
+ IsConstant && Result.isSigned() && (Subtract != Result.isNegative());
+ bool IsConstZero = IsConstant && !Result.getBoolValue();
+ if (UB && (IsConstZero ||
+ (TestIsLessOp ? (IsConstNeg || (IsUnsigned && Subtract))
+ : (!IsConstNeg || (IsUnsigned && !Subtract))))) {
+ Actions.Diag(NewStep->getExprLoc(),
+ diag::err_omp_loop_incr_not_compatible)
+ << Var << TestIsLessOp << NewStep->getSourceRange();
+ Actions.Diag(ConditionLoc,
+ diag::note_omp_loop_cond_requres_compatible_incr)
+ << TestIsLessOp << ConditionSrcRange;
+ return true;
+ }
+ }
----------------
Richard Smith wrote:
> Hmm, you give an error if the increment is a constant and allow it if it's
> not. I suppose that makes some sense, but it seems really weird. The rule
> from the OpenMP spec is extremely vague about this...
Yes, if the increment is not constant, we have to assume that it is
loop-invariant and that it is compatible with loop condition, we'll need to
calculate it once before the loop.
================
Comment at: lib/Sema/SemaOpenMP.cpp:989-991
@@ +988,5 @@
+ // var = lb
+ // integer-type var = lb
+ // random-access-iterator-type var = lb
+ // pointer-type var = lb
+ //
----------------
Richard Smith wrote:
> Is this really supposed to be a syntactic check? It bans reasonable things
> like:
>
> #pragma omp simd(...)
> for (int (*p)[4] = lb; p != lb + 8; ++p)
Well, in this case init is accepted.
In some cases it would be also possible to allow '!=' in condition ( e.g., as
operation here is '++', we would expect '<' instead of '!=' ), under some
ext-warning. Though, I would prefer to not allow it for now.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1027-1028
@@ +1026,4 @@
+ }
+ if (auto Init = dyn_cast<CXXConstructExpr>(Var->getInit())) {
+ if (Init->getNumArgs() != 1) {
+ Actions.Diag(InitLoc,
----------------
Richard Smith wrote:
> This isn't the right check: you should look at `Var->getInitStyle()` to check
> that the declaration was of a form involving an `=` (but that seems like a
> bizarre and unnecessary rule, so maybe this should be an `ExtWarn`). As this
> stands, you'll accept braced and parenthesized initialization, and do the
> wrong thing in the presence of default constructors with default arguments.
OK, changed it to ext-warning.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1034
@@ +1033,3 @@
+ }
+ return SetVarAndLB(Var, Init->getArg(0));
+ }
----------------
Richard Smith wrote:
> This seems strange, and possibly incorrect. The argument might be of a type
> entirely unrelated to the iterator type, and may not provide the lower bound
> for the iteration. Why are you pulling out the first argument here?
Fixed.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1054
@@ +1053,3 @@
+/// \brief Ignore parenthesises, implicit casts, copy constructor and return
the
+/// variable (which may be loop variable) if possible.
+static const VarDecl *GetInitVarDecl(const Expr *E) {
----------------
Richard Smith wrote:
> "may be the loop variable"
Fixed.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1151
@@ +1150,3 @@
+ }
+ bool IsIncrement = BO->getOpcode() == BO_Add;
+ if (GetInitVarDecl(BO->getLHS()) == Var)
----------------
Richard Smith wrote:
> `IsAdd` would make more sense; we generally use "increment" to mean only `++`.
OK
================
Comment at: lib/Sema/SemaOpenMP.cpp:1157
@@ +1156,3 @@
+ } else if (auto CE = dyn_cast<CXXOperatorCallExpr>(RHS)) {
+ bool IsIncrement = CE->getOperator() == OO_Plus;
+ if (!IsIncrement && CE->getOperator() != OO_Minus) {
----------------
Richard Smith wrote:
> Likewise.
OK
================
Comment at: lib/Sema/SemaOpenMP.cpp:1163
@@ +1162,3 @@
+ }
+ assert(CE->getNumArgs() == 2);
+ if (GetInitVarDecl(CE->getArg(0)) == Var)
----------------
Richard Smith wrote:
> This looks wrong: both `+` and `-` can be overloaded as unary operators.
Fixed.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1291-1294
@@ +1290,6 @@
+ // For C, a variable of a pointer type.
+ QualType VarType = Var->getType()
+ .getNonReferenceType()
+ .getCanonicalType()
+ .getUnqualifiedType();
+ if (!VarType->isDependentType() && !VarType->isIntegerType() &&
----------------
Richard Smith wrote:
> Don't canonicalize the type here. Also, why are you stripping off references?
> The spec doesn't imply that you should do that as far as I can see.
OK
================
Comment at: lib/Sema/SemaOpenMP.cpp:1342
@@ +1341,3 @@
+
+static Stmt *IgnoreContainerStmts(Stmt *S, bool IgnoreCaptured) {
+ if (IgnoreCaptured) {
----------------
Richard Smith wrote:
> Do you have some reference to the OpenMP spec to justify this?
I've added a comment.
It's a helper routine to skip braces around for stmt, to get a nested for stmt,
for example in the following case:
#pragma omp simd collapse(2)
for (int i = 0; i < n; ++i) {
for (int j = 0; j < n; ++j)
foo();
}
================
Comment at: lib/Sema/SemaOpenMP.cpp:1360-1389
@@ +1359,32 @@
+
+namespace {
+/// \brief Checker for EH throw/try/catch stmt absense.
+class EhChecker : public StmtVisitor<EhChecker, bool> {
+ llvm::SmallVector<Stmt *, 8> EhStmts;
+
+public:
+ bool VisitCXXCatchStmt(CXXCatchStmt *S) {
+ EhStmts.push_back(S);
+ VisitStmt(S);
+ return true;
+ }
+ bool VisitCXXThrowExpr(CXXThrowExpr *S) {
+ EhStmts.push_back(S);
+ return true;
+ }
+ bool VisitCXXTryStmt(CXXTryStmt *S) {
+ EhStmts.push_back(S);
+ VisitStmt(S);
+ return true;
+ }
+ bool VisitStmt(Stmt *S) {
+ bool SeenEh = false;
+ for (auto I : S->children())
+ SeenEh |= (I && Visit(I));
+ return SeenEh;
+ }
+ EhChecker() {}
+ llvm::SmallVector<Stmt *, 8> getEhStmts() { return EhStmts; }
+};
+}
+
----------------
Richard Smith wrote:
> Please check this when building these statements, as you do for `break`,
> rather than performing an additional visit of them after the fact.
OK
http://reviews.llvm.org/D3778
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
