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

Tue Dec 1 07:33:51 PST 2020

jdoerfert added a comment.

While only partially related, can you leave a FIXME saying that more than 15 arguments need to be packed in a structure?

================
Comment at: clang/test/OpenMP/parallel_codegen.cpp:139
 // CHECK:       call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i{{64|32}})* [[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], i{{64|32}} %{{.+}})
-// IRBUILDER:   call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i{{64|32}})* [[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], i{{64|32}} %{{.+}})
+// IRBUILDER:   call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, i{{64|32}}*, i8***)* [[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i{{64|32}}* %{{.+}}, i8*** [[ARGC_ADDR]])
 // ALL:  ret i32 0
----------------
ftynse wrote:
> The order of arguments changes here because we create a use of the promoted-to-pointer argument before any other uses in the body and the outliner finds it first. This should be fine because it's just an internal outlined function that the builder created and the calls to it are emitted accordingly and in the same builder. I can add a comment that explains this if desired.
> 
> If we go with Michael's suggestion not to turn into pointers the integer values whose size is equal to or smaller than pointer size, this change will not be necessary. I seem to remember seeing some documentation that says that trailing arguments to `fork_call` should be _pointers_, but I can't find it anymore.
>  I seem to remember seeing some documentation that says that trailing arguments to fork_call should be _pointers_, but I can't find it anymore.

Technically, anything that is passed in the same register as a `void *` is fine. The documentation on this is thin at best. As I mentioned in my response to @Meinersbur, I think turning everything into pointers is a reasonable way to handle this. I gave some rational there (https://reviews.llvm.org/D92189#2424690)

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:736
              "Expected copy/create callback to set replacement value!");
-      if (ReplacementValue == &V)
-        return;
     }

----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92189