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