[PATCH] D99459: [OpenMP] Implement '#pragma omp unroll'.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur marked 8 inline comments as done.
Meinersbur added inline comments.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:463-478
+  static const SpecificClause *getSingleClause(ArrayRef<OMPClause *> Clauses) {
+    auto ClausesOfKind = getClausesOfKind<SpecificClause>(Clauses);
 
-    if (Clauses.begin() != Clauses.end()) {
-      assert(std::next(Clauses.begin()) == Clauses.end() &&
+    if (ClausesOfKind.begin() != ClausesOfKind.end()) {
+      assert(std::next(ClausesOfKind.begin()) == ClausesOfKind.end() &&
              "There are at least 2 clauses of the specified kind");
+      return *ClausesOfKind.begin();
----------------
ABataev wrote:
> Better to commit it it separately as NFC.
D103665


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:10139-10142
+/// Find and diagnose mutually exclusive clause kinds.
+static bool checkMutuallyExclusiveClauses(
+    Sema &S, ArrayRef<OMPClause *> Clauses,
+    ArrayRef<OpenMPClauseKind> MutuallyExclusiveClauses) {
----------------
ABataev wrote:
> Would be good if this change is committed separately as NFC.
D103666


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12821
+/// Determine whether an expression is constant without emitting diagnostic.
+static bool isConstantExpression(Sema &SemaRef, Expr *E) {
+  struct ConstTripcountDiagnoser : public Sema::VerifyICEDiagnoser {
----------------
ABataev wrote:
> Can you use `VerifyPositiveIntegerConstantInClause` function instead?
The reason for this new function is that `VerifyPositiveIntegerConstantInClause` also emits a note on why it is not a constant expression:
```
note: read of non-const variable '.capture_expr.' is not allowed in a constant expression
```
Printing internal variable names is more confusing than helpful to the user.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12964
+  assert(Factor > 0 && "Expected positive unroll factor");
+  auto makeFactorExpr = [this, Factor, IVTy, FactorLoc]() {
+    return IntegerLiteral::Create(
----------------
ABataev wrote:
> Why do you need it? Just use original `PartialClause->getFactor()`
The AST invariant that no ASTNode can be used multiple times (within the same DeclContext) must be preserved. This is a utility lambda (like the other `MakeXyz`) that create a new IntegerLiteral node for ever use.

See discussion here: https://reviews.llvm.org/D94973?id=322600#inline-905341


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99459/new/

https://reviews.llvm.org/D99459