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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I only have small nits, nothing major. @fghanim, is this OK with you, if so, please accept?



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:106
                         BasicBlock &ContinuationBB)>;
+  // This is created primarily for sections construct as llvm::function_ref
+  // are not storable
----------------
Newline. Reference BodyGenCallBackTy for argument explanation.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:879
+    // have a terminator, which is already removed by EmitOMPRegionBody.
+    if (IP.getBlock()->end() == IP.getPoint()) {
+      // IP is currently at cancelation block.
----------------
Prefer early exits 


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:891
+      IP = InsertPointTy(I->getParent(), I->getIterator());
+    }
+    return FiniCB(IP);
----------------
No backtracking, and similar schemes, if you can just remember the blocks you are looking for when you create them.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:897
+
+  // Each section in emitted as a switch case
+  // Each finalization callback is handled from clang.EmitOMPSectionDirective()
----------------



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:988
+      IP = InsertPointTy(I->getParent(), I->getIterator());
+    }
+    return FiniCB(IP);
----------------
same comments as above, early exists & remember the blocks rather than looking for them


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89671