[PATCH] [OPENMP] Loop canonical form analysis (Sema)

[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;
+}
+
----------------
Alexander Musman wrote:
> hfinkel at anl.gov wrote:
> > Richard Smith wrote:
> > > 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?
> > 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.
> > 
> 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.
> 
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.)

================
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;
+  }
+
----------------
Alexander Musman wrote:
> Richard Smith wrote:
> > Maybe check this when creating the `BreakStmt` itself? (Clear the `BreakScope` flag in your OpenMP loop statement's `Scope`.)
> How do you think, it is worth adding a bit in Scope, to modify error message (this seems to me a rare enough case)?
> There is already an "OpenMPDirectiveScope" bit, but not all openmp directives prohibit break stmts.
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.)

http://reviews.llvm.org/D3778