[PATCH] D89671: [LLVM][OpenMP] Adding support for OpenMP sections construct in OpenMPIRBuilder

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some drive-by comments. I will assume @fghanim continues the review, feel free to not wait on me.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:52
+  OMP_sch_modifier_nonmonotonic = (1 << 30),
+};
 /// An interface to create LLVM-IR for OpenMP directives.
----------------
This should go into OMPKinds.def or the tablegen file.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:395
   ///
-  /// \returns The insertion position *after* the master.
+  /// \returns The insertion position *after* the critical.
   InsertPointTy CreateCritical(const LocationDescription &Loc,
----------------
Commit things like this without review as NFC changes.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:416
+                               FinalizeCallbackTy FiniCB, bool IsCancellable,
+                               bool IsNoWait);
+
----------------
Is nowait has no doxygen comment above. I would prefer `IsNowait` as spelling.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:797
+
+  InsertPointTy CurrIP = Builder.saveIP();
+  Builder.restoreIP(AllocaIP);
----------------
Nit: `CurIP` (to match the other spelling above).


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:879
+
+  // emit kmpc_for
+  // TODO: Change the following to call the IR Builder function to emit kmpc_for
----------------
Comments, here and elsewhere, should be proper sentences.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:906
+    CreateBarrier(Builder, OMPD_sections, true, IsCancellable);
+  }
+
----------------
No braces.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1135
+                   llvm::Value &Val, llvm::Value *&ReplVal) {
+    // TODO: Privatization not implemented in OMPIRBuilder yet
+    return CodeGenIP;
----------------
FWIW, tt never will be, that is a front-end task.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89671