[Openmp-commits] [PATCH] D82470: [OpenMP][IRBuilder] Support nested parallel regions

[Fady Ghanim via Phabricator via Openmp-commits]

fghanim added a comment.

Thanks for the Patch. I have few questions to help me understand what's going on.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:282
+  IRBuilder<> AllocaBuilder;
+
   /// Map to remember source location strings
----------------
What's the benefit of this over just maintaining an alloca insertion point?

With this we will have 3 `IRBuilder`s to maintain and keep track of: the clang (or flang) `IRBuilder`, the OMP `IRBuilder` and the `allocaBuilder`


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:294
     PostOutlineCBTy PostOutlineCB;
+    BasicBlock *EntryBB, *ExitBB;
+
----------------
I see two benefits of passing entry and exit blocks, as opposed to what we used to do:
1. less memory, but in return we collect blocks twice (i.e. O(N) mem & O(N+E) work vs O(1) mem and 2 * O(N+E) work ). Do you expect that the vector is likely to become large enough where it is a problem? if not, what's the benefit of the change?

2. If some blocks are added later, then this becomes a correctness issue. Which is unlikely since it happens after the body codegen is complete. However, if I am mistaken, shouldn't we also delay searching for inputs/outputs?


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:299
+    void collectBlocks(SmallPtrSetImpl<BasicBlock *> &BlockSet,
+                       SmallVectorImpl<BasicBlock *> &BlockVector);
   };
----------------
What is the benefit of passing `blockSet` when it is exclusively used inside of `collectBlocks`? 
I don't think I saw a usage of it in calling functions. am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82470