This patch implements semantic analysis to make sure that the loop is in OpenMP canonical form.
This is the form required for 'omp simd', 'omp for' and other loop pragmas.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
| lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 1222–1281 ↗ | (On Diff #9418) | Wow. I really don't think we should be computing this here. It also seems wrong, since you presumably want to know whether you have contiguously-stored elements, not whether you have random access iterators, right? |
| 1451–1457 ↗ | (On Diff #9418) | Maybe check this when creating the BreakStmt itself? (Clear the BreakScope flag in your OpenMP loop statement's Scope.) |
| lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 1222–1281 ↗ | (On Diff #9418) | No, section 2.6 of the OpenMP spec specifically requires a random-access iterator type for the iteration variable; specifically: var One of the following: A variable of a signed or unsigned integer type. For C++, a variable of a random access iterator type. For C, a variable of a pointer type. |
Comment Actions
Comment at: lib/Sema/SemaOpenMP.cpp:1451-1457
@@ +1450,9 @@
+ // Perform conformance checks of the loop body.
+ ForBreakStmtChecker BreakCheck;
+ if (CurStmt && BreakCheck.Visit(CurStmt)) {
+ for (auto Break : BreakCheck.getBreakStmts())
+ Diag(Break->getLocStart(), diag::err_omp_loop_cannot_break)
+ << getOpenMPDirectiveName(DKind);
+ return true;
+ }+
Maybe check this when creating the BreakStmt itself? (Clear the BreakScope flag in your OpenMP loop statement's Scope.)
Richard,
But then we will see some bad kind of diagnostic, like "'break'
statement not in loop or switch statement", though actually there is a
loop statement. That's quite confusing.
Best regards,
Alexey Bataev
Software Engineer
Intel Compiler Team
Intel Corp.
23.05.2014 4:32, Richard Smith пишет:
Comment at: lib/Sema/SemaOpenMP.cpp:1222-1281
@@ +1221,62 @@
+
+/ \brief Return true if the given type is pointer or other random access
+/ iterator (currently only std-compatible iterators are allowed here).
+bool Sema::IsRandomAccessIteratorType(QualType Ty, SourceLocation Loc) {
+ if (Ty->isPointerType())
+ return true;
+ if (!getLangOpts().CPlusPlus || !Ty->isOverloadableType())
+ return false;
+ Check that var type is a random access iterator, i.e. we can apply
+ 'std::distance' to the init and test arguments of the for-loop.
+ CXXScopeSpec StdScopeSpec;
+ StdScopeSpec.Extend(Context, getOrCreateStdNamespace(), SourceLocation(),
+ SourceLocation());
+
+ TemplateDecl *TIT = nullptr;
+ {
+ DeclarationNameInfo DNI(&Context.Idents.get("iterator_traits"),
+ SourceLocation());
+ LookupResult LR(*this, DNI, LookupNestedNameSpecifierName);
+ if (!LookupParsedName(LR, DSAStack->getCurScope(), &StdScopeSpec) ||
+ !LR.isSingleResult() || !(TIT = LR.getAsSingle<TemplateDecl>()))
+ return false;
+ }
+
+ CXXRecordDecl *IT = nullptr;
+ {
+ TemplateArgumentListInfo Args;
+ TemplateArgument Arg(Ty);
+ TemplateArgumentLoc ArgLoc(Arg, Context.CreateTypeSourceInfo(Ty));
+ Args.addArgument(ArgLoc);
+ QualType T = CheckTemplateIdType(TemplateName(TIT), SourceLocation(), Args);
+ if (T.isNull() || RequireCompleteType(Loc, T, 0) ||
+ !(IT = T->getAsCXXRecordDecl()))
+ return false;
+ }
+
+ TypeDecl *RAI = nullptr;
+ {
+ DeclarationNameInfo DNI(&Context.Idents.get("random_access_iterator_tag"),
+ SourceLocation());
+ LookupResult LR(*this, DNI, LookupOrdinaryName);
+ CXXRecordDecl *RDType = Ty->getAsCXXRecordDecl();
+ if (!LookupParsedName(LR, DSAStack->getCurScope(), &StdScopeSpec) ||
+ !LR.isSingleResult() || !(RAI = LR.getAsSingle<TypeDecl>()) || !RDType)
+ return false;
+ }
+
+ TypeDecl *IC = nullptr;
+ {
+ DeclarationNameInfo DNI(&Context.Idents.get("iterator_category"),
+ SourceLocation());
+ LookupResult LR(*this, DNI, LookupOrdinaryName);
+ if (!LookupQualifiedName(LR, IT) || !LR.isSingleResult() ||
+ !(IC = LR.getAsSingle<TypeDecl>()) ||
+ !Context.hasSameType(Context.getTypeDeclType(RAI),
+ Context.getTypeDeclType(IC)))
+ return false;
+ }
+
+ return true;
+}+
Wow. I really don't think we should be computing this here. It also seems wrong, since you presumably want to know whether you have contiguously-stored elements, not whether you have random access iterators, right?
Comment at: lib/Sema/SemaOpenMP.cpp:1451-1457
@@ +1450,9 @@
+ // Perform conformance checks of the loop body.
+ ForBreakStmtChecker BreakCheck;
+ if (CurStmt && BreakCheck.Visit(CurStmt)) {
+ for (auto Break : BreakCheck.getBreakStmts())
+ Diag(Break->getLocStart(), diag::err_omp_loop_cannot_break)
+ << getOpenMPDirectiveName(DKind);
+ return true;
+ }+
Maybe check this when creating the BreakStmt itself? (Clear the BreakScope flag in your OpenMP loop statement's Scope.)
Comment Actions
Richard,
Thank you for review.
| lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 1222–1281 ↗ | (On Diff #9418) | Basically it would be OK to have as a loop counter any type, which value at any iteration can be calculated in constant time using "begin" and the current iteration's number (of some integer type), if we knew how to calculate it. As noted by Hal, it is explicitly allowed by OpenMP 4 for random access iterators - for example, it specifies that the type in which the loop count is: "For C++, if var is of a random access iterator type, then the type is the type that would be used by std::distance applied to variables of the type of var.". This is why we need a check that there is std::distance. |
| 1451–1457 ↗ | (On Diff #9418) | How do you think, it is worth adding a bit in Scope, to modify error message (this seems to me a rare enough case)? |
| lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 1222–1281 ↗ | (On Diff #9418) | OK. This still seems strange to me, but I agree that the OpenMP spec appears to want this. The next question is, what does it mean by "a random access iterator type"? In the C++ standard, that does not mean "std::iterator_traits<T>::iterator_category is derived from std::random_access_iterator_tag". It means that the type conforms to the requirements of a random access iterator (see 24.2.7 [random.access.iterators] p1). And that's not actually a computable property. Is a diagnostic really required here, or should we just *assume* that the iteration variable is a random access iterator, use the operations which the random access iterator requirements require to exist, and produce the relevant diagnostics if they didn't work? (That's how range-based for loops work, for instance.) |
| 1451–1457 ↗ | (On Diff #9418) | Adding a bit to Scope to indicate that it is an OpenMP scope that prohibits break statements seems fine to me. (Be careful to bump Flags from unsigned short to unsigned int at the same time, and reorder it before Depth to avoid unnecessary padding.) |
Comment Actions
Richard,
Yes, I think that assuming that the variable is random access iterator would work. It will still need to generate std::difference on them, but we can check that later, during building expressions for iteration space (that will be used for CodeGen). This also has an advantage that user may use his iterators without deriving from std classes.
I've removed corresponding checks and fixed processing break statements.
Thanks,
Alexander
| include/clang/Basic/DiagnosticSemaKinds.td | ||
|---|---|---|
| 6961 ↗ | (On Diff #9875) | What does "dsa" stand for? |
| 6963–6975 ↗ | (On Diff #9875) | 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' |
| 6964 ↗ | (On Diff #9875) | Please write this as OpenMP, not as openmp. |
| 6976–6979 ↗ | (On Diff #9875) | How about replacing both of these with: condition of OpenMP for loop must be a relational comparison ('<', '<=', '>', or '>=') of loop variable %0 |
| 6980–6985 ↗ | (On Diff #9875) | How about: increment clause of OpenMP for loop must perform simple addition or subtraction on loop variable %0 |
| 6988–6989 ↗ | (On Diff #9875) | Maybe increment step of OpenMP for loop is of non-integer type %0 |
| 6991–6992 ↗ | (On Diff #9875) | Please mention in the diagnostic that the reason for this rule is that this is an OpenMP for loop. |
| 6995–6998 ↗ | (On Diff #9875) | Perhaps replace both of these with %q0 statement cannot be used in OpenMP for loop |
| include/clang/Sema/Sema.h | ||
| 7316–7322 ↗ | (On Diff #9875) | It looks like neither of these needs to be a member of Sema: make them static functions inside SemaOpenMP instead? |
| lib/Sema/SemaOpenMP.cpp | ||
| 819 ↗ | (On Diff #9875) | 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. |
| 842 ↗ | (On Diff #9875) | This comment is strangely-worded. |
| 852 ↗ | (On Diff #9875) | Please capitalize these references to other members properly, and refer to them as #Var and #LB (that's what makes Doxygen happy, apparently). |
| 866 ↗ | (On Diff #9875) | "Check the right-hand side of an assignment in the increment expression." |
| 882–886 ↗ | (On Diff #9875) | This is all redundant. Just use Var->getType()->isDependentType(). |
| 888–891 ↗ | (On Diff #9875) | 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. |
| 892–899 ↗ | (On Diff #9875) | Copy-paste error here. I assume you intended to check Step? |
| 935–937 ↗ | (On Diff #9875) | I think you only care about whether NewStep is value-dependent here. |
| 945 ↗ | (On Diff #9875) | You've already issued a diagnostic for this case inside PerformImplicitIntegerConversion. |
| 961–976 ↗ | (On Diff #9875) | 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... |
| 989–991 ↗ | (On Diff #9875) | 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) |
| 1027–1028 ↗ | (On Diff #9875) | 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. |
| 1034 ↗ | (On Diff #9875) | 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? |
| 1054 ↗ | (On Diff #9875) | "may be the loop variable" |
| 1151 ↗ | (On Diff #9875) | IsAdd would make more sense; we generally use "increment" to mean only ++. |
| 1157 ↗ | (On Diff #9875) | Likewise. |
| 1163 ↗ | (On Diff #9875) | This looks wrong: both + and - can be overloaded as unary operators. |
| 1291–1294 ↗ | (On Diff #9875) | 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. |
| 1342 ↗ | (On Diff #9875) | Do you have some reference to the OpenMP spec to justify this? |
| 1360–1389 ↗ | (On Diff #9875) | Please check this when building these statements, as you do for break, rather than performing an additional visit of them after the fact. |
| include/clang/Basic/DiagnosticSemaKinds.td | ||
|---|---|---|
| 6961 ↗ | (On Diff #9875) | Data-sharing Attributes (term from OpenMP, section 2.14.1). I moved this to be near other _dsa error diagnostic messages. |
| 6963–6975 ↗ | (On Diff #9875) | OK |
| 6976–6979 ↗ | (On Diff #9875) | OK |
| 6980–6985 ↗ | (On Diff #9875) | OK |
| 6988–6989 ↗ | (On Diff #9875) | I removed this message - it turns out, this is already checked by PerformImplicitIntegerConversion. |
| 6991–6992 ↗ | (On Diff #9875) | OK |
| 6995–6998 ↗ | (On Diff #9875) | 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. |
| include/clang/Sema/Sema.h | ||
| 7316–7322 ↗ | (On Diff #9875) | Fixed. |
| lib/Sema/SemaOpenMP.cpp | ||
| 819 ↗ | (On Diff #9875) | Fixed. There are actually several other 'Actions' in SemaOpenMP.cpp unrelated to my change, I plan to fix them in separate commit. |
| 842 ↗ | (On Diff #9875) | Changed to "This flag is true when step is subtracted on each iteration." |
| 852 ↗ | (On Diff #9875) | OK |
| 866 ↗ | (On Diff #9875) | OK |
| 882–886 ↗ | (On Diff #9875) | OK |
| 888–891 ↗ | (On Diff #9875) | OK |
| 892–899 ↗ | (On Diff #9875) | Yes, this was a bug. |
| 935–937 ↗ | (On Diff #9875) | Yes, fixed. |
| 945 ↗ | (On Diff #9875) | OK |
| 961–976 ↗ | (On Diff #9875) | 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. |
| 989–991 ↗ | (On Diff #9875) | Well, in this case init is accepted. |
| 1027–1028 ↗ | (On Diff #9875) | OK, changed it to ext-warning. |
| 1034 ↗ | (On Diff #9875) | Fixed. |
| 1054 ↗ | (On Diff #9875) | Fixed. |
| 1151 ↗ | (On Diff #9875) | OK |
| 1157 ↗ | (On Diff #9875) | OK |
| 1163 ↗ | (On Diff #9875) | Fixed. |
| 1291–1294 ↗ | (On Diff #9875) | OK |
| 1342 ↗ | (On Diff #9875) | I've added a comment. #pragma omp simd collapse(2)
for (int i = 0; i < n; ++i) {
for (int j = 0; j < n; ++j)
foo();
} |
| 1360–1389 ↗ | (On Diff #9875) | OK |
Comment Actions
Hi Richard,
Thank you for review. I believe I've fixed all your comments.
Best regards,
Alexander
| include/clang/Sema/Scope.h | ||
|---|---|---|
| 399–406 ↗ | (On Diff #10012) | Please either handle this by copying the OpenMPSimdDirectiveScope to relevant child scopes in Scope::Init or by adding an OpenMPSimdDirectiveParent (if you actually need to know which parent scope had the directive). Also, this loop doesn't look entirely correct: I would expect that simd-ness shouldn't propagate into (say) local classes declared within an OpenMP simd directive: void f() {
#pragma omp simd ...
for (...) {
struct S {
void g() { throw 0; }
};
}
}... but I'm not sure what the OpenMP spec says about this case. |
| include/clang/Sema/Sema.h | ||
| 7264 ↗ | (On Diff #10012) | Should this have a name involving OpenMP? It's not obvious from the declaration that this isn't a general-purpose utility. |
| lib/Sema/SemaOpenMP.cpp | ||
| 1018–1020 ↗ | (On Diff #10012) | No braces around a single-line if body. |
| 1270–1271 ↗ | (On Diff #10012) | What's this for? If HasErrors is true here, then we've found and diagnosed a problem despite some part of the loop being dependent, and we should return true. |
| include/clang/Sema/Scope.h | ||
|---|---|---|
| 399–406 ↗ | (On Diff #10012) | The OpenMP spec does not cover such cases, but yes, it seems reasonable to allow EH here. |
| include/clang/Sema/Sema.h | ||
| 7264 ↗ | (On Diff #10012) | OK, I've renamed it to PerformOpenMPImplicitIntegerConversion |
| lib/Sema/SemaOpenMP.cpp | ||
| 1018–1020 ↗ | (On Diff #10012) | Fixed |
| 1270–1271 ↗ | (On Diff #10012) | I agree, fixed. |
Comment Actions
LGTM
| lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 1269–1270 ↗ | (On Diff #10043) | You could remove this, and the function Dependent itself. I guess maybe you're keeping it because you'll want this check once you implement the FIXME below? If so, this is fine to leave here. |
| test/OpenMP/simd_loop_messages.cpp | ||
| 474–475 ↗ | (On Diff #10043) | You can drop the 1s here. |