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

[Alexey Bataev via Phabricator via llvm-commits]

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