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

Mon Nov 30 21:42:40 PST 2020

jdoerfert requested changes to this revision.
jdoerfert added a subscriber: ggeorgakoudis.
jdoerfert added a comment.
This revision now requires changes to proceed.

Nit: Clang tidy warning wrt variable names should be addressed. Some other nits inlined.

My only concern is described in the longer inline comment.

---

In D92189#2424216 <https://reviews.llvm.org/D92189#2424216>, @fghanim wrote:

> To be honest, I am not exactly sure what is the problem you are trying to address here, is it about passing shared variables as proper pointers? or is it about creating locals for said arguments

The problem was discussed in the Flang + OpenMP call lately and got attention by at least 4 people since then, as far as I can tell independently. I'm rather confused TBH, but it's a "good thing", as far as bug fixes can be good ;)

Long story short, `kmpc_fork_call` expects variadic arguments to be in integer registers, anything else passed is lost and replaced with garbage. Clang deals with this already but since the problem is not unique to Clang we should do it here. If anyone wants to create a parallel region they should not need to know the implicit calling convention rules of the OpenMP runtime. For an example how clang "optimizes" things, see https://godbolt.org/z/cja5TE, especially the float/i32 bitcasts.

> Based on the patch description; I think it is the first; and AFAIK, the current implementation will always pass shared variables correctly as pointers. is that not the case? I mean I have never came across a case where it didn't
> If yes, do you have a c/c++ example I can try? I'd be very interested in seeing for which cases this doesn't work.

It's not about shared variables but basically the equivalent of "firstprivate" (=pass by value), though it occurs in non-frontend contexts as well, e.g. when merging/expanding parallel regions.

> However, implementation and checks makes me think it is the second, Which I always assumed was going to be a part of how a specific frontend handles privatization; but I am very much not against doing so here

It belongs here and I asked them to put it here instead of Flang, especially since @ggeorgakoudis exposed the same problem in OpenMP opt. Now instead of fixing up pointers 3 times and exposing a very fragile API to non-OpenMP users, we will ensure the calling convention is not leaking out of the IRBuilder.

> Just will want to make sure to do it in a way that works for the OMP-IR, and I don't know if there is or what is the plan to handle private/shared variables is on their end.

Privatization (in all its glory) has still to happen in the frontend. This is only partially related.

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

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

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:754
+    LLVM_DEBUG(llvm::dbgs() << "Forwarding input as pointer: " << V << "\n");
+    assert(Reloaded);
+
----------------
Nit: Assertion message please.

================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:774
+    // because we all uses of the original value in the to-be-outlined region
+    // must disappear.
+    if (ReplacementValue == &V)
----------------
Nit: -"we"

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92189