[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

Tue Dec 17 15:10:39 PST 2019

jdoerfert added a comment.

In D70258#1788427 <https://reviews.llvm.org/D70258#1788427>, @rjmccall wrote:

> In D70258#1788396 <https://reviews.llvm.org/D70258#1788396>, @jdoerfert wrote:
>
> > In D70258#1788305 <https://reviews.llvm.org/D70258#1788305>, @rjmccall wrote:
> >
> > > Introducing an IRBuilder-level finalization stack sounds like it's going to be a huge mess if your goal is to plug this into other frontends.
> >
> >
> > While I get that you don't want to review this, I would really like to understand why you think this would become a mess.
>
>
> I guess it depends on what you're expecting to be able to achieve with this stack.  Frontends have their own notion of what needs to be finalized and what can trigger control flow.  If your finalization stack is purely for the convenience of your internal IR-generation, it's fine.  If the need for a finalizer can cross the emission of arbitrary frontend code, or if your code needs to emit branches that might cross arbitrary frontend "scope" boundaries, you're going to be in trouble.

Let me explain what it does and why, maybe that helps:

The "finalizer" is a frontend provided callback as only the frontend "knows" what values to finalize and "how" to finalize them if we leave the scope. It is created only for OpenMP scopes as we only manage leaving those in the OMPBuilder. If we generate code for an OpenMP directive that will cause control flow to leave the region the finalizer is invoked. Other language finalization, e.g., because of nested control flow, will happen undisturbed. It works because OpenMP restricts what can happen in a region, e.g. you cannot return from an OpenMP parallel region, so all exits have to be "natural" or OpenMP related.

FWIW: We already have (basically) the same scheme in Clang, managed by the CGOpenMP and doing basically the same thing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258