[PATCH] D74372: [OpenMP][IRBuilder] Perform finalization (incl. outlining) late

Fady Ghanim via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 11 13:36:46 PST 2020


fghanim added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:113
+  if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder())
+    OMPBuilder->finalize();
 }
----------------
jdoerfert wrote:
> fghanim wrote:
> > jdoerfert wrote:
> > > fghanim wrote:
> > > > Does this mean that `finalize()` is being called twice everytime, here and `~OpenMPIRBuilder()`?
> > > > if yes, is there a reason why you want that?
> > > It's called twice now, not that it matters. The basic rule is, call it at least once before you finalize the module. I'll remove the destructor for now to make it explicit only.
> > Right now it doesn't matter, I agree. However, if later `finalize()` is modified for any reason, I worry this may introduce problems unintentionally.
> > 
> > In any case, if you decide to keep only one of them, I prefer to keep the one in `~OpenMPIRBuilder()`. This way the `OMPBuilder` is self contained, so regardless of the frontend using it, we can be certain it's always called at the end. You can indicate explicitly in the description comment for `finalize()` that it is being called in the destructor, if you want.
> That doesn't work, otherwise I would not call finalize here. At least it doesn't work without extra changes. As it is right now, the OMPBuilder in CodeGenModule is deleted *after* the IR is generated and emitted, so any changes at destructor time are too late.
Got it. Then a minor suggestion is to just put a note in `finalize()` description about whatever you decide to do (i.e. keep both, remove the one in destructor, or something else entirely). This way, whoever is using/modifying the code is aware of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74372





More information about the cfe-commits mailing list