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

plotfi added a comment. In D74372#1873665 , @jdoerfert wrote: > In D74372#1873623 , @plotfi wrote: > > > Could this be reverted for the time being so that bots can go green? There > > appear to be an awful lot of

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

jdoerfert added a comment. In D74372#1873623 , @plotfi wrote: > Could this be reverted for the time being so that bots can go green? There > appear to be an awful lot of commits piling up on top of this one. Should be fixed by now but you should revert

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

plotfi added a comment. Could this be reverted for the time being so that bots can go green? There appear to be an awful lot of commits piling up on top of this one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74372/new/

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

vvereschaka added a comment. it also gets failed on the Linux builders http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/2843 http://lab.llvm.org:8011/builders/lld-x86_64-ubuntu-fast/builds/11581 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

vvereschaka added a comment. Hello @jdoerfert, `OpenMPIRBuilderTest.ParallelCancelBarrier` is failed on Windows builders: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-win-fast/builds/11242/steps/test-check-llvm-unit/logs/stdio

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG8a56d64d7620: [OpenMP][IRBuilder] Perform finalization (incl. outlining) late (authored by jdoerfert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74372/new/ https://reviews.llvm.org/D74372

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

jdoerfert added a comment. I think this fixes the bug and allows loop optimizations later, let me know if there is anything else Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74372/new/ https://reviews.llvm.org/D74372

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

vsk added inline comments. Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1408 - // See llvm.org/PR44560, OpenMP passes an invalid subprogram to CodeExtractor. - bool NeedWorkaroundForOpenMPIRBuilderBug = - OldSP && OldSP->getRetainedNodes()->isTemporary(); -

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

jdoerfert updated this revision to Diff 244005. jdoerfert added a comment. Improve comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74372/new/ https://reviews.llvm.org/D74372 Files: clang/lib/CodeGen/CodeGenFunction.cpp

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

jdoerfert added a comment. In D74372#1870620 , @vsk wrote: > Thanks :). IIRC `check-clang` is enough to exercise the relevant code path, > as we get an assert in CodeExtractor without the workaround. (side note: > please don't take my comments here as

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

jdoerfert updated this revision to Diff 244002. jdoerfert marked 6 inline comments as done. jdoerfert added a comment. Removed OMPIRBuilder workaround in extractor and addressed comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74372/new/

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

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:

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

vsk added a comment. Thanks :). IIRC `check-clang` is enough to exercise the relevant code path, as we get an assert in CodeExtractor without the workaround. (side note: please don't take my comments here as blocking, I just wanted to see if we could delete some cruft) Repository: rG LLVM

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

jdoerfert added a comment. In D74372#1870547 , @vsk wrote: > Can this delete `NeedWorkaroundForOpenMPIRBuilderBug` from > llvm/lib/Transforms/Utils/CodeExtractor.cpp? I didn't know about the workaround but that was the plan. I'll verify and add it to

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

vsk added a comment. Can this delete `NeedWorkaroundForOpenMPIRBuilderBug` from llvm/lib/Transforms/Utils/CodeExtractor.cpp? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74372/new/ https://reviews.llvm.org/D74372

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

jdoerfert marked 2 inline comments as done. jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:113 + if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) +OMPBuilder->finalize(); } fghanim wrote: > jdoerfert

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

fghanim added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:113 + if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) +OMPBuilder->finalize(); } jdoerfert wrote: > fghanim wrote: > > Does this mean that `finalize()` is

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

jdoerfert marked 7 inline comments as done. jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:113 + if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) +OMPBuilder->finalize(); } fghanim wrote: > Does this

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

fghanim added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:113 + if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) +OMPBuilder->finalize(); } Does this mean that `finalize()` is being called twice everytime, here and

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

JonChesterfield added a comment. Patch looks good with the above nits. I'm not totally sure about the callback vs running a separate IR pass after the finalize() call, but when the callback is this simple it looks fine. I like that this preserves the current semantics.

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

rogfer01 added inline comments. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:114 + +// Add some known attributes to the outlined function. +Function *OutlinedFn = Extractor.extractCodeRegion(CEAC); This comment seems misplaced now.

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

jdoerfert created this revision. jdoerfert added reviewers: rogfer01, ABataev, JonChesterfield, kiranchandramohan, fghanim. Herald added subscribers: cfe-commits, guansong, bollu, hiraditya. Herald added projects: clang, LLVM. In order to fix PR44560 and to prepare for loop transformations we