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

[Alex Zinenko via Phabricator via cfe-commits]

ftynse planned changes to this revision.
ftynse added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:736
              "Expected copy/create callback to set replacement value!");
-      if (ReplacementValue == &V)
-        return;
     }
 
----------------
jdoerfert wrote:
> ftynse wrote:
> > 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.
> Appreciate the detailed explanations!
> 
> > 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.
> 
> This looks like a sweet-spot. We can avoid adding more replacement logic but the `PrivCB` is aware of what is going on. If you are OK with this solution as well I'd prefer it.
> 
Great, will do this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92189