Page MenuHomePhabricator

[OpenMP][IRBuilder] Perform finalization (incl. outlining) late
ClosedPublic

Authored by jdoerfert on Mon, Feb 10, 6:19 PM.

Details

Summary

In order to fix PR44560 and to prepare for loop transformations we now
finalize a function late, which will also do the outlining late. The
logic is as before but the actual outlining step happens now after the
function was fully constructed. Once we have loop transformations we
can apply them in the finalize step before the outlining.

Diff Detail

Event Timeline

jdoerfert created this revision.Mon, Feb 10, 6:19 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMon, Feb 10, 6:19 PM
rogfer01 added inline comments.Mon, Feb 10, 11:29 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
114

This comment seems misplaced now.

659–660

I think you forgot to wrap these in a LLVM_DEBUG

666

This used to be called AfterIP. Probably the exact name is not that important but the unit tests still use AfterIP so I'd suggest to keep both names in sync.

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.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
272

Calling getNewOutlineInfo will invalidate the references previously returned. That's not a bug in this patch but looks like it'll be easy to get wrong in future.

Perhaps the backing store should be a linked list, such that push_back doesn't invalidate any existing references?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
668

s/parallel/outlined?

fghanim added inline comments.Tue, Feb 11, 6:59 AM
clang/lib/CodeGen/CodeGenFunction.cpp
113

Does this mean that finalize() is being called twice everytime, here and ~OpenMPIRBuilder()?
if yes, is there a reason why you want that?

jdoerfert marked 7 inline comments as done.Tue, Feb 11, 7:52 AM
jdoerfert added inline comments.
clang/lib/CodeGen/CodeGenFunction.cpp
113

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.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
272

The address of these objects is not stable but without that requirement we don't need a list. We make this function take a OutlineInfo object that we than move into the vector if that is preferred.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
114

Agreed, will be moved.

659–660

I simplify forgot the remove them. Thx for noticing.

666

I'll call it AfterIP again.

668

this is in the omp parallel callback so "parallel" is still fine.

fghanim added inline comments.Tue, Feb 11, 12:10 PM
clang/lib/CodeGen/CodeGenFunction.cpp
113

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.

jdoerfert marked 2 inline comments as done.Tue, Feb 11, 12:56 PM
jdoerfert added inline comments.
clang/lib/CodeGen/CodeGenFunction.cpp
113

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.

vsk added a subscriber: vsk.Tue, Feb 11, 12:59 PM

Can this delete NeedWorkaroundForOpenMPIRBuilderBug from llvm/lib/Transforms/Utils/CodeExtractor.cpp?

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 the patch.

vsk added a comment.Tue, Feb 11, 1:21 PM

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)

fghanim added inline comments.Tue, Feb 11, 1:31 PM
clang/lib/CodeGen/CodeGenFunction.cpp
113

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.

jdoerfert updated this revision to Diff 244002.Tue, Feb 11, 2:15 PM
jdoerfert marked 6 inline comments as done.

Removed OMPIRBuilder workaround in extractor and addressed comments

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 blocking, I just wanted to see if we could delete some cruft)

Can you verify I deleted the workaround? I was not sure if the workaround is all of it or only the OMP... part.
I run the test you mentioned w/ and w/o the debug flag you mentioned in the PR, both times it passes w/o assertion:

PASS: Clang :: OpenMP/parallel_codegen.cpp
jdoerfert updated this revision to Diff 244005.Tue, Feb 11, 2:22 PM

Improve comment

vsk added inline comments.Tue, Feb 11, 2:30 PM
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1408

This part lgtm, thanks.

I think this fixes the bug and allows loop optimizations later, let me know if there is anything else

This revision is now accepted and ready to land.Wed, Feb 12, 3:05 PM
This revision was automatically updated to reflect the committed changes.

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
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/4537
http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/4534

C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\llvm\unittests\Frontend\OpenMPIRBuilderTest.cpp(612): error:       Expected: CI->getNextNode()->getSuccessor(0)

      Which is: 000000DC6479BEC0

To be equal to: ExitBB

      Which is: 000000DC6479B140

[  FAILED  ] OpenMPIRBuilderTest.ParallelCancelBarrier (4 ms)

Would you you fix the test or revert this commit?

plotfi added a subscriber: plotfi.Wed, Feb 12, 10:09 PM

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.

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 if you feel it causes a problem.

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 if you feel it causes a problem.

Nice, thank you! https://github.com/llvm/llvm-project/commit/3f3ec9c40b2574929b41b93ac38484081b49837b did the trick.