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