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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function<void(InsertPointTy /* CodeGenIP */)>;
+
----------------
jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > `llvm::function_ref`?
> > > > > > > The lambda that is passed in here might go out of scope so we need to own it. This is intentional.
> > > > > > Maybe better to save the intermediate data in CGOpenMPRuntime class rather than rely on owning lambda ref here? Clang does not use escaping decls and instead stores intermediate data explicitly. It really complicates analysis and potential source of resource leakage.
> > > > > I don't follow. Clang does use `std::function` to store callbacks that have to life for a while. Why is this different? What would be the benefit of having a function_ref here and a `std::function` in CGOpenMPRuntime? Note that the FinaliztionInfo object is created in the front-end and the std::function is assigned there already.
> > > > Clang uses it only in some rare cases, when it is really required, like typo correction or something like this. In other cases it is not recommended to use it.
> > > > 
> > > > I'm not saying that you need to store `std::function` in CGOpenMPRuntime class, I'm saying about necessary data.
> > > What necessary data? Can you please explain how you want it to look like and why? This version seems to work fine.
> > I'm not arguing that it does not work, I'm asking do you really need such a complex solution? Just like I said before, it is very hard to maintain and to understand the lifetime of lambda, so it is a potential source of resource leakage. All I'm asking is `is there a way to implement it differently`, nothing else.
> There are alternatives, all of which are more complex and come with the same "problems".
@jdoerfert , can you please elaborate? What other designs might be possible?

It looks to me like this lambda is necessary to maintain separation between Clang's CodeGen and the OpenMPIRBuilder (the non-unit-test use in this patch only captures CGF). I'm not particularly concerned about lifetime management of the lambdas if they only need to capture CGF, and maybe the design of this implies that the lambdas will only capture objects that live as long as the code generation phase, but it is certainly true that whenever we have long-lived lambdas thought about lifetime of captured data is required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258