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

[Fady Ghanim via Phabricator via llvm-commits]

fghanim added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:387
+                               FinalizeCallbackTy FiniCB, bool IsCancellable,
+                               bool IsNowait);
+
----------------
Nit: add Doxygen documentation for `IsNoWait`


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:798
+  FinalizationStack.push_back(
+      {FiniCB, OMPD_sections, /*IsCancellable*/ IsCancellable});
+
----------------
you only need to use the stack for outlined regions - for inlined regions (e.g. section/sections) you don't.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:912
+         "Unexpected Directive for Finalization call!");
+  Fi.FiniCB(InsertPointTy(SwitchExitBB, SwitchExitBB->getFirstInsertionPt()));
+
----------------
What I meant by my comment; is that you will use the same finiCB multiple times - once per section. you should undo the last changes, However, no need to do add anything back, but based on your clang impl. this is not the place for it.
every `sections` directive has one or more `section` directive, and based on clang impl. that thing is generated separately.  The finalization block will be generated inside the `section` directive.

Implementation-wise; this translates to writing a `createSection` which can be implemented as an inlined region, look at `createCritical` for an example of how to do it. The clang side is also quite easy - again look at `emitOMPCritical` in clang for an example.

I am **not ** going to require that you write `createSection` along with its clang usage to accept this patch, **unless ** the clang lit test for `omp sections` doesn't work without it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89671