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

[Fady Ghanim via Phabricator via cfe-commits]

fghanim added a comment.

Thanks; just two more mediocre things if possible. if not. you are good to go. :)

@jdoerfert

In D92189#2424677 <https://reviews.llvm.org/D92189#2424677>, @jdoerfert wrote:

> In D92189#2424216 <https://reviews.llvm.org/D92189#2424216>, @fghanim wrote:
>
>> 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
>
> The problem was discussed in the Flang + OpenMP call lately and got attention by at least 4 people since then, as far as I can tell independently. I'm rather confused TBH, but it's a "good thing", as far as bug fixes can be good ;)

I don't disagree. As I said, "I am very much not against doing so here"



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:697
+  IRBuilder<>::InsertPoint ReloadIP(ZeroAddrUse->getParent(),
+                                    ZeroAddrUse->getNextNode()->getIterator());
+
----------------
ftynse wrote:
> fghanim wrote:
> > Why not use `InnerAllocaIP` instead? it's in the entry block of the parallel region.
> I started with that, but it did not work out. `InnerAllocaIP` is used by `PrivCB` to construct IR that may be using the value defined at `ReloadIP`. If I literally replace the use of `ReloadIP` with `InnerAllocaIP`, the instructions with have the wrong order. I also considered inserting this at the top of the entry block, but this messes up the order of arguments in the outlined function. Suggestions on how to structure this better are welcome.
OK; The only instructions generated at `InnerAllocaIP` up to this point are the things generated in `bodyCB`, are these the users that you are talking about?

In any case, and since you already reset `InnerAllocaIP` to ` ReloadIP`, I prefer we do that before privatization (i.e. reset `InnerAllocaIP`, to `ZeroAddrUse->getNextNode()` directly), and use that instead. 


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:716
+      Builder.SetInsertPoint(InsertBB,
+                             InsertBB->getTerminator()->getIterator());
+      Value *Ptr = Builder.CreateAlloca(V.getType());
----------------
ftynse wrote:
> fghanim wrote:
> > 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`
> This belongs to the outer function, but `OuterAllocaIP` does not seem valid at this point anymore. Using it leads to segfaults, I can investigate later, after we decide whether this code should remain here or move below as Johannes suggested.
That's probably because the `OuterAllocaIP` iterator got invalidated between the beginning and here.

If you want; get the insertion BasicBlock early on, and then use that to reupdate `OuterAllocaIP` and use that to place whatever you want. right now those alloca.s are not in an entry block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92189