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

This revision was automatically updated to reflect the committed changes. Closed by commit rGf9c3c5da19ab: [OpenMP][IR-Builder] Introduce the finalization stack (authored by jdoerfert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https:

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

ABataev accepted this revision. ABataev added a comment. This revision is now accepted and ready to land. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 ___

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

rjmccall added a comment. Okay, if this is just handling OpenMP-specific control flow and doesn't need to interact with what a reasonable frontend would do with cleanups, I'm appeased. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ http

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

jdoerfert added a comment. In D70258#1788861 , @rjmccall wrote: > That's what I figured. Just to say this again: Current OpenMP code generation keeps a second stack for exactly the same purpose as the one introduced here. > The thing is that that neces

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

jdoerfert added a comment. In D70258#1788825 , @rjmccall wrote: > So it's never that OpenMP has things that need to be finalized before exiting > (e.g. if something in frontend-emitted code throws an exception), it's just > that OpenMP might need to gene

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

rjmccall added a comment. That's what I figured. The thing is that that necessarily interacts correctly with everything in Clang's IRGen in ways that making a second stack that Clang's IRGen doesn't directly know about doesn't. I think you either need to extract Clang's entire cleanup-stack co

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

ABataev added a comment. In D70258#1788825 , @rjmccall wrote: > So it's never that OpenMP has things that need to be finalized before exiting > (e.g. if something in frontend-emitted code throws an exception), it's just > that OpenMP might need to gene

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

rjmccall added a comment. So it's never that OpenMP has things that need to be finalized before exiting (e.g. if something in frontend-emitted code throws an exception), it's just that OpenMP might need to generate its own control flow that breaks through arbitrary scopes in the frontend? I wo

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

jdoerfert added a comment. In D70258#1788427 , @rjmccall wrote: > In D70258#1788396 , @jdoerfert wrote: > > > In D70258#1788305 , @rjmccall > > wrote: > > > > > Introducing

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

rjmccall added a comment. In D70258#1788396 , @jdoerfert wrote: > In D70258#1788305 , @rjmccall wrote: > > > Introducing an IRBuilder-level finalization stack sounds like it's going to > > be a huge mess if your go

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

jdoerfert added a subscriber: rjmccall. jdoerfert added a comment. In D70258#1788305 , @rjmccall wrote: > I opposed the creation of this library on these terms in the first place, so > I'm pretty sure I'm not on the hook to review it. That's fine with m

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

rjmccall resigned from this revision. rjmccall added a comment. I opposed the creation of this library on these terms in the first place, so I'm pretty sure I'm not on the hook to review it. Introducing an IRBuilder-level finalization stack sounds like it's going to be a huge mess if your goal

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

jdoerfert added a comment. ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

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

jdoerfert marked an inline comment as done. jdoerfert 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; + hfinkel wrote: >

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

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; + jdoerfert wrote: > ABataev wrote: > > jdoerfert wrote: > > > A

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

jdoerfert marked an inline comment as done. jdoerfert 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; + ABataev wrote: >

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

ABataev 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; + jdoerfert wrote: > ABataev wrote: > > jdoerfert wrote: > > > A

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

jdoerfert marked an inline comment as done. jdoerfert 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; + ABataev wrote: >

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

ABataev 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; + jdoerfert wrote: > ABataev wrote: > > jdoerfert wrote: > > > A

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

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 60744 tests passed, 0 failed and 726 were skipped. {icon check-circle color=green} clang-format: pass. Build artifacts : console-log.txt

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

jdoerfert marked an inline comment as done. jdoerfert 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; + ABataev wrote: >

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

jdoerfert updated this revision to Diff 233472. jdoerfert marked an inline comment as done. jdoerfert added a comment. Update the unit test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 Files: clang/lib

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

ABataev 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; + jdoerfert wrote: > ABataev wrote: > > `llvm::function_ref`? >

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

jdoerfert marked 2 inline comments as done. jdoerfert added a comment. In D70258#1780611 , @ABataev wrote: > Tests? This changes the implementation and does not add features. Thus, this is covered by the existing tests for the functionality, e.g. the un

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

merge_guards_bot added a comment. {icon check-circle color=green} clang-format: pass. Build artifacts : console-log.txt , CMakeCache.txt <

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

ABataev added a comment. Tests? Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51 + /// at the time, and location, the callback is invoked. + using FinalizeCallbackTy = std::function; + `llvm::function_ref`? Repository: rG LLVM Github Monore

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

jdoerfert updated this revision to Diff 233462. jdoerfert added a comment. rebase + ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 Files: clang/lib/CodeGen/CGOpenMPRuntime.cpp llvm/include/llvm/Fr

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

jdoerfert marked 4 inline comments as done. jdoerfert added a comment. ping Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71 + void pushFinalizationCB(FinalizationInfo &&FI) { +FinalizationStack.emplace_back(std::move(FI)); + } jdoerfert wro

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

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71 + void pushFinalizationCB(FinalizationInfo &&FI) { +FinalizationStack.emplace_back(std::move(FI)); + } ABataev wrote:

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

jdoerfert updated this revision to Diff 229582. jdoerfert marked an inline comment as done. jdoerfert added a comment. Make it a specialized push-pop RAII object (just moved code) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://rev

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

ABataev added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1481-1509 + if (OMPBuilder) { + +// The following callback is the crucial part of clangs cleanup process. +// +// NOTE: +// Once the OpenMPIRBuilder is used to create parallel region

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

ABataev added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71 + void pushFinalizationCB(FinalizationInfo &&FI) { +FinalizationStack.emplace_back(std::move(FI)); + } jdoerfert wrote: > ABataev wrote: > > jdoerfert wrote: > > >

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

jdoerfert updated this revision to Diff 229573. jdoerfert added a comment. Replace conditional by RAII object Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 Files: clang/lib/CodeGen/CGOpenMPRuntime.cpp

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

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71 + void pushFinalizationCB(FinalizationInfo &&FI) { +FinalizationStack.emplace_back(std::move(FI)); + } ABataev wrote:

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

ABataev added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1499-1509 +OMPBuilder->pushFinalizationCB(std::move(FI)); + } + CGOpenMPOutlinedRegionInfo CGInfo(*CS, ThreadIDVar, CodeGen, InnermostKind, HasCancel, Out

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

jdoerfert updated this revision to Diff 229448. jdoerfert marked 5 inline comments as done. jdoerfert added a comment. Addressed comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 Files: clang/lib/C

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

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1499-1509 +OMPBuilder->pushFinalizationCB(std::move(FI)); + } + CGOpenMPOutlinedRegionInfo CGInfo(*CS, ThreadIDVar, CodeGen, InnermostKind, HasCancel, O

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

ABataev added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1483 +// OpenMPIRBuilder is asked to construct a parallel (or similar) construct. +auto FiniCB = [&](llvm::OpenMPIRBuilder::InsertPointTy IP) { + assert(IP.getBlock()->end() == IP.getPoi

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

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added subscribers: cfe-commits, guansong, bollu, hiraditya. Herald added projects: clang, LLVM. As a permanent and gene