[PATCH] [OPENMP] Loop canonical form analysis (Sema)
Richard Smith
richard at metafoo.co.uk
Thu May 29 22:20:54 PDT 2014
================
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">;
----------------
What does "dsa" stand for?
================
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<
----------------
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'
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6964
@@ +6963,3 @@
+def err_omp_loop_not_canonical_init : Error<
+ "initialization of the openmp loop should be assignment to "
+ "(or definition of) the loop variable">;
----------------
Please write this as OpenMP, not as openmp.
Use 'must', not 'should'; this is a hard requirement.
================
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<
----------------
How about replacing both of these with:
condition of OpenMP for loop must be a relational comparison ('<', '<=', '>', or '>=') of loop variable %0
================
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<
----------------
How about:
increment clause of OpenMP for loop must perform simple addition or subtraction on loop variable %0
================
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<
----------------
Maybe
increment step of OpenMP for loop is of non-integer type %0
================
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<
----------------
Please mention in the diagnostic that the reason for this rule is that this is an OpenMP for loop.
================
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
----------------
Perhaps replace both of these with
%q0 statement cannot be used in OpenMP for loop
================
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,
----------------
It looks like neither of these needs to be a member of `Sema`: make them `static` functions inside SemaOpenMP instead?
================
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).
----------------
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.
================
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;
----------------
This comment is strangely-worded.
================
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);
----------------
Please capitalize these references to other members properly, and refer to them as #Var and #LB (that's what makes Doxygen happy, apparently).
================
Comment at: lib/Sema/SemaOpenMP.cpp:866
@@ +865,3 @@
+private:
+ /// \brief Check right-hand side of the increment expression.
+ bool CheckIncRHS(Expr *RHS);
----------------
"Check the right-hand side of an assignment in the increment expression."
================
Comment at: lib/Sema/SemaOpenMP.cpp:882-886
@@ +881,7 @@
+ }
+ QualType VarType = Var->getType()
+ .getNonReferenceType()
+ .getCanonicalType()
+ .getUnqualifiedType();
+ if (VarType->isDependentType())
+ return true;
----------------
This is all redundant. Just use `Var->getType()->isDependentType()`.
================
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 &&
----------------
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.
================
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;
----------------
Copy-paste error here. I assume you intended to check `Step`?
================
Comment at: lib/Sema/SemaOpenMP.cpp:935-937
@@ +934,5 @@
+ return true;
+ if (!NewStep->isValueDependent() && !NewStep->isTypeDependent() &&
+ !NewStep->isInstantiationDependent() &&
+ !NewStep->containsUnexpandedParameterPack()) {
+
----------------
I think you only care about whether `NewStep` is value-dependent here.
================
Comment at: lib/Sema/SemaOpenMP.cpp:945
@@ +944,3 @@
+ else {
+ Actions.Diag(NewStep->getExprLoc(), diag::err_omp_loop_incr_not_integer);
+ return true;
----------------
You've already issued a diagnostic for this case inside `PerformImplicitIntegerConversion`.
================
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;
+ }
+ }
----------------
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...
================
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
+ //
----------------
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)
================
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,
----------------
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.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1034
@@ +1033,3 @@
+ }
+ return SetVarAndLB(Var, Init->getArg(0));
+ }
----------------
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?
================
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) {
----------------
"may be the loop variable"
================
Comment at: lib/Sema/SemaOpenMP.cpp:1151
@@ +1150,3 @@
+ }
+ bool IsIncrement = BO->getOpcode() == BO_Add;
+ if (GetInitVarDecl(BO->getLHS()) == Var)
----------------
`IsAdd` would make more sense; we generally use "increment" to mean only `++`.
================
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) {
----------------
Likewise.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1163
@@ +1162,3 @@
+ }
+ assert(CE->getNumArgs() == 2);
+ if (GetInitVarDecl(CE->getArg(0)) == Var)
----------------
This looks wrong: both `+` and `-` can be overloaded as unary operators.
================
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() &&
----------------
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.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1342
@@ +1341,3 @@
+
+static Stmt *IgnoreContainerStmts(Stmt *S, bool IgnoreCaptured) {
+ if (IgnoreCaptured) {
----------------
Do you have some reference to the OpenMP spec to justify this?
================
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; }
+};
+}
+
----------------
Please check this when building these statements, as you do for `break`, rather than performing an additional visit of them after the fact.
http://reviews.llvm.org/D3778
More information about the cfe-commits
mailing list