[PATCH] [OPENMP] Loop canonical form analysis (Sema)
Alexander Musman
alexander.musman at gmail.com
Mon Jun 2 04:28:58 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">;
----------------
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
More information about the cfe-commits
mailing list