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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:697
+  IRBuilder<>::InsertPoint ReloadIP(ZeroAddrUse->getParent(),
+                                    ZeroAddrUse->getNextNode()->getIterator());
+
----------------
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.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:716
+      Builder.SetInsertPoint(InsertBB,
+                             InsertBB->getTerminator()->getIterator());
+      Value *Ptr = Builder.CreateAlloca(V.getType());
----------------
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.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:736
              "Expected copy/create callback to set replacement value!");
-      if (ReplacementValue == &V)
-        return;
     }
 
----------------
jdoerfert wrote:
> jdoerfert wrote:
> > I was expecting the above logic to be placed here, after the `PrivCB` callback as I assumed we would privatize in the sequential part. Clang does not and we do not either (for now), which is unfortunate. It does duplicate the privatization and makes this patch more complicated than I anticipated. I though we can simply replace `ReplacementValue` by `Reloaded` if `ReplacementValue` wasn't a pointer and therefor put in an alloca. The fact that we need to reload `V` and then "replace twice" seems really unfortunate. 
> > 
> > What would happen if you do not have `Reloaded` at all but instead make it `V = createlodoad(alloca)`? After all, `Reloaded` is equivalent to `V` in all aspects, right? Would this work? Would we still need the code below? I feel like there must be a minimal solution as we only want to replace the value once on the other side of the call edge.
> -"duplicate privatization" +"duplicate replace all uses with"
I am not sure I fully follow what you suggest, so I will elaborate on the two versions I had considered.

1. Move the code that loads back the value (currently lines 709-725) after this line.  This will not remove the need for two separate "replace all uses with": there uses of the original `V` in the body that need to be replaced with `ReplacementValue`, and there are uses of `V` that could have been introduced by `PrivCB` for the purpose of //defining// `ReplacementValue` which should be replaced with `Reloaded` instead. This doesn't look like it would address your concern about having double replacement.

2. I can keep the code that loads back the value in its current place and pass either `V` or `*Reloaded` to `PrivCB`. This will make sure any instructions created in `PrivCB` use the `Reloaded` and don't need to be update via "replace all uses with" pattern. However, this exposes the pointer mechanism to the caller of `CreateParallel`. The `Value &` that `PrivCB` receives is not necessarily a value that exists in the user code, it can be the alloca we constructed in builder. So the implementation of `PrivCB` needs to be aware of it and can no longer rely on, e.g., keeping a list of values/instructions that need privatization or directly rely on types since the type would change. I decided that I did not want to change the contract that `PrivCB` has with `CreateParallel` because I was not aware of all its use cases (and have a preference for more "isolated" APIs) . However, if you think it is acceptable for the builder in order to reduce the complexity/overhead of the code, I can adapt.

There is a flavor of (2) that changes `PrivCB` to take both `V` and `Replacement` so that the implementation of `PrivCB` can easily detect when the pointer mechanism is active.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92189