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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
