[PATCH] D92189: [OpenMPIRBuilder] forward arguments as pointers to outlined function

[Fady Ghanim via Phabricator via llvm-commits]

fghanim added a reviewer: fghanim.
fghanim added a comment.

Thank you for the patch :)

To be honest, I am not exactly sure what is the problem you are trying to address here, is it about passing shared variables as proper pointers? or is it about creating locals for said arguments

Based on the patch description; I think it is the first; and AFAIK, the current implementation will always pass shared variables correctly as pointers. is that not the case? I mean I have never came across a case where it didn't
If yes, do you have a c/c++ example I can try? I'd be very interested in seeing for which cases this doesn't work.

However, implementation and checks makes me think it is the second, Which I always assumed was going to be a part of how a specific frontend handles privatization; but I am very much not against doing so here
Just will want to make sure to do it in a way that works for the OMP-IR, and I don't know if there is or what is the plan to handle private/shared variables is on their end.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:697
+  IRBuilder<>::InsertPoint ReloadIP(ZeroAddrUse->getParent(),
+                                    ZeroAddrUse->getNextNode()->getIterator());
+
----------------
Why not use `InnerAllocaIP` instead? it's in the entry block of the parallel region.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:711
+    if (!V.getType()->isPointerTy()) {
+      auto currentIP = Builder.saveIP();
+
----------------
NIT: IPGuard maybe better?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:716
+      Builder.SetInsertPoint(InsertBB,
+                             InsertBB->getTerminator()->getIterator());
+      Value *Ptr = Builder.CreateAlloca(V.getType());
----------------
For the alloca, should use `OuterAllocaIP` or `InnerAllocaIP` based on which function the alloca belongs to.

allocations in InsertBB will end right before the fork call, which suggests you probably should use outerAllocaIP. but why do you need two allocations for same variable. or is this meant to be the allocations where we store the arguments in the outlined function? if that's the case, then you should use `InnerAllocaIP`


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:717
+                             InsertBB->getTerminator()->getIterator());
+      Value *Ptr = Builder.CreateAlloca(V.getType());
+      Builder.CreateStore(&V, Ptr);
----------------
Nit: if you could specify a name for the alloca, a name is easier to track than a number in IR :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92189