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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits