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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:907
+  /// \param C   Context of the AST.
+  /// \param Loc Location of the 'full' identifier.
+  static OMPFullClause *Create(const ASTContext &C, SourceLocation StartLoc,
----------------
Fix params descriptions


================
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();
----------------
Better to commit it it separately as NFC.


================
Comment at: clang/lib/AST/StmtOpenMP.cpp:382
+                           Stmt *PreInits) {
+  OMPUnrollDirective *Dir = createDirective<OMPUnrollDirective>(
+      C, Clauses, AssociatedStmt, TransformedStmtOffset + 1, StartLoc, EndLoc);
----------------
`auto *`


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2594-2598
+  if (S.hasClausesOfKind<OMPFullClause>())
+    LoopStack.setUnrollState(LoopAttributes::Full);
+
+  auto *PartialClause = S.getSingleClause<OMPPartialClause>();
+  if (PartialClause) {
----------------
Can we have `full` and `partial` at the same time? If no, emit partial in `else` substatement for `full` clause.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:8984-8985
+              DependentPreInits = Dir->getPreInits();
+            } else
+              llvm_unreachable("Unexpected loop transformation");
             if (!DependentPreInits)
----------------
Enclose in braces


================
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) {
----------------
Would be good if this change is committed separately as NFC.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12604
+      });
+  assert(OriginalInits.back().empty());
+  OriginalInits.pop_back();
----------------
assert message?


================
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 {
----------------
Can you use `VerifyPositiveIntegerConstantInClause` function instead?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12955-12956
+  SourceLocation FactorLoc;
+  if (auto *FactorConst =
+          cast_or_null<ConstantExpr>(PartialClause->getFactor())) {
+    Factor = FactorConst->getResultAsAPSInt().getZExtValue();
----------------
Better to use `PartialClause->getFactor()->isIntegerConstantExpr(Ctx)` or something similar.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12964
+  assert(Factor > 0 && "Expected positive unroll factor");
+  auto makeFactorExpr = [this, Factor, IVTy, FactorLoc]() {
+    return IntegerLiteral::Create(
----------------
Why do you need it? Just use original `PartialClause->getFactor()`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12985
+      buildVarDecl(*this, {}, IVTy, OuterIVName, nullptr, OrigVar);
+  auto makeOuterRef = [this, OuterIVDecl, IVTy, OrigVarLoc]() {
+    return buildDeclRefExpr(*this, OuterIVDecl, IVTy, OrigVarLoc);
----------------
`MakeOuterRef`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12993
+  InnerIVDecl->setDeclName(&PP.getIdentifierTable().get(InnerIVName));
+  auto makeInnerRef = [this, InnerIVDecl, IVTy, OrigVarLoc]() {
+    return buildDeclRefExpr(*this, InnerIVDecl, IVTy, OrigVarLoc);
----------------
`MakeInnerRef`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:13000
+  CaptureVars CopyTransformer(*this);
+  auto makeNumIterations = [&CopyTransformer, &LoopHelper]() -> Expr * {
+    return AssertSuccess(
----------------
`MakeNumIterations`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99459