ABataev added inline comments.
================
Comment at: clang/include/clang/AST/OpenMPClause.h:899
+public:
+ /// Build a 'sizes' AST node.
+ ///
----------------
`sizes`->`full`
================
Comment at: clang/include/clang/AST/OpenMPClause.h:902
+ /// \param C Context of the AST.
+ /// \param StartLoc Location of the 'sizes' identifier.
+ /// \param LParenLoc Location of '('.
----------------
Same
================
Comment at: clang/include/clang/AST/OpenMPClause.h:903
+ /// \param StartLoc Location of the 'sizes' identifier.
+ /// \param LParenLoc Location of '('.
+ /// \param EndLoc Location of ')'.
----------------
No param
================
Comment at: clang/include/clang/AST/OpenMPClause.h:904
+ /// \param LParenLoc Location of '('.
+ /// \param EndLoc Location of ')'.
+ /// \param Sizes Content of the clause.
----------------
Wrong description
================
Comment at: clang/include/clang/AST/OpenMPClause.h:905
+ /// \param EndLoc Location of ')'.
+ /// \param Sizes Content of the clause.
+ static OMPFullClause *Create(const ASTContext &C, SourceLocation StartLoc,
----------------
No param
================
Comment at: clang/include/clang/AST/OpenMPClause.h:912
+ /// \param C Context of the AST.
+ /// \param NumSizes Number of items in the clause.
+ static OMPFullClause *CreateEmpty(const ASTContext &C);
----------------
No param
================
Comment at: clang/include/clang/AST/OpenMPClause.h:946
+public:
+ /// Build a 'sizes' AST node.
+ ///
----------------
sizes->partial
================
Comment at: clang/include/clang/AST/OpenMPClause.h:948-952
+ /// \param C Context of the AST.
+ /// \param StartLoc Location of the 'sizes' identifier.
+ /// \param LParenLoc Location of '('.
+ /// \param EndLoc Location of ')'.
+ /// \param Sizes Content of the clause.
----------------
Fix params descriptions
================
Comment at: clang/include/clang/AST/OpenMPClause.h:957-960
+ /// Build an empty 'sizes' AST node for deserialization.
+ ///
+ /// \param C Context of the AST.
+ /// \param NumSizes Number of items in the clause.
----------------
Description
================
Comment at: clang/include/clang/AST/OpenMPClause.h:971
+
+ void setFactor(Expr *E) { Factor = E; }
+
----------------
Make it private, if possible
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:5037
+/// This represents the '#pragma omp tile' loop transformation directive.
+class OMPUnrollDirective final : public OMPLoopBasedDirective {
----------------
Description
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:5061-5072
+ /// Create a new AST node representation for '#pragma omp tile'.
+ ///
+ /// \param C Context of the AST.
+ /// \param StartLoc Location of the introducer (e.g. the 'omp' token).
+ /// \param EndLoc Location of the directive's end (e.g. the tok::eod).
+ /// \param Clauses The directive's clauses.
+ /// \param NumLoops Number of associated loops (number of items in the
----------------
Fix description
================
Comment at: clang/include/clang/AST/StmtOpenMP.h:5078-5082
+ /// Build an empty '#pragma omp tile' AST node for deserialization.
+ ///
+ /// \param C Context of the AST.
+ /// \param NumClauses Number of clauses to allocate.
+ /// \param NumLoops Number of associated loops to allocate.
----------------
Description
================
Comment at: clang/lib/AST/StmtProfile.cpp:466
void OMPClauseProfiler::VisitOMPSizesClause(const OMPSizesClause *C) {
- for (auto E : C->getSizesRefs())
+ for (Expr *E : C->getSizesRefs())
if (E)
----------------
Unelated change?
================
Comment at: clang/lib/AST/StmtProfile.cpp:474
+void OMPClauseProfiler::VisitOMPPartialClause(const OMPPartialClause *C) {
+ if (Expr *Factor = C->getFactor())
+ Profiler->VisitExpr(Factor);
----------------
`const Expr *`
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2573
+
+ auto FullClauses = S.getClausesOfKind<OMPFullClause>();
+ const OMPFullClause *FullClause = nullptr;
----------------
`getSingleClause` or `hasClausesOfKind`
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2580
+
+ auto PartialClauses = S.getClausesOfKind<OMPPartialClause>();
+ const OMPPartialClause *PartialClause = nullptr;
----------------
`getSingleClause`
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2589
+ if (PartialClause) {
+ if (Expr *FactorExpr = PartialClause->getFactor()) {
+ RValue FactorRVal = EmitAnyExpr(FactorExpr, AggValueSlot::ignored(),
----------------
`const Expr *`
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2590-2593
+ RValue FactorRVal = EmitAnyExpr(FactorExpr, AggValueSlot::ignored(),
+ /*ignoreResult=*/true);
+ Factor =
+ cast<llvm::ConstantInt>(FactorRVal.getScalarVal())->getZExtValue();
----------------
I suppose it is compiled-time expression, right? If so, use
`FactorExpr->EvaluateKnownConstInt()` or something similar.
================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2948-2949
break;
default:
- break;
+ llvm_unreachable("Unhandled clause");
}
----------------
Better to remove the `default:` case completely
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12451
+bool Sema::checkTransformableLoopNest(
+ OpenMPDirectiveKind Kind, Stmt *AStmt, int NumLoops,
----------------
I think this function can be made static local as it is used only SemaOpenMP.cpp
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12691
+ auto FullClauses =
+ OMPExecutableDirective::getClausesOfKind<OMPFullClause>(Clauses);
+ const OMPFullClause *FullClause = nullptr;
----------------
`getSingleClause`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12694
+ if (!FullClauses.empty()) {
+ assert(hasSingleElement(FullClauses));
+ FullClause = *FullClauses.begin();
----------------
Add a text message to the assert.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12699
+ auto PartialClauses =
+ OMPExecutableDirective::getClausesOfKind<OMPPartialClause>(Clauses);
+ const OMPPartialClause *PartialClause = nullptr;
----------------
`getSingleClause`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12717
+ Stmt *TransformedStmt = nullptr;
+ // Stmt* PreInits = nullptr;
+
----------------
Need to remove
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12724-12726
+ if (!checkTransformableLoopNest(OMPD_tile, AStmt, NumLoops, LoopHelpers,
Body,
+ OriginalInits))
+ return StmtError();
----------------
I think this should be checked before the first `OMPUnrollDirective::Create`
call
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12800
+
+ // Create tile loops from the inside to the outside.
+ for (int I = NumLoops - 1; I >= 0; --I) {
----------------
unroll?
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:14728
+ case OMPC_partial:
+ Res = ActOnOpenMPPartialClause(nullptr, StartLoc, {}, EndLoc);
+ break;
----------------
Need to pass factor expr and real source loc
================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:3179
+ unsigned NumLoops = Record[ASTStmtReader::NumStmtFields];
+ assert(NumLoops == 1);
+ unsigned NumClauses = Record[ASTStmtReader::NumStmtFields + 1];
----------------
Add a message for the assert
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
