Page MenuHomePhabricator

[LLVM][OpenMP] Adding support for OpenMP sections construct in OpenMPIRBuilder
ClosedPublic

Authored by AMDChirag on Oct 19 2020, 12:39 AM.

Details

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
AMDChirag marked an inline comment as not done.Nov 9 2020, 1:36 AM
AMDChirag added inline comments.
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
959

So, for this, the basic blocks of cases should have the finiCB callback called too?
Something like

auto CaseBB = CreateBB();
SetInsertPoint(CaseBB);
SectionCB(...);
FiniCB(...);

One idea: Now with D90830 landed, CreateSections could be implemented by calling that (with TripCount == Number of sections), then call "CreateWorkshareLoop" (the same that we would use to implement #pragma omp for) to it, this would allow us to share the implementations of worksharing-loop and sections.

CreateWorkshareLoop does not exist yet, we might refactor this code once it exists.

I second @Meinersbur comment. Reducing duplication is always better :)

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

The only time where you will require multiple different FiniCB, is if the code to emit cleanup code for different sections is different, which -unless i am missing something- i don't think is the case - for Clang at least. So it's very likely that Having one finiCB defined and reusing it for each region is going to work

959

yes, but you may need to create a block (e.g. FiniBB) reachable from caseBB, and pass it to FiniCB to emit code there.

you can check createMaster, createSingle, createCritical for an example

AMDChirag updated this revision to Diff 305773.Nov 17 2020, 6:47 AM
AMDChirag marked 3 inline comments as done.

Added FiniCB usage and Control Flow Graph

AMDChirag marked 6 inline comments as done.Nov 17 2020, 6:48 AM
fghanim added inline comments.Nov 18 2020, 10:06 PM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
704

Nit: add Doxygen documentation for IsNoWait

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

you only need to use the stack for outlined regions - for inlined regions (e.g. section/sections) you don't.

1009

What I meant by my comment; is that you will use the same finiCB multiple times - once per section. you should undo the last changes, However, no need to do add anything back, but based on your clang impl. this is not the place for it.
every sections directive has one or more section directive, and based on clang impl. that thing is generated separately. The finalization block will be generated inside the section directive.

Implementation-wise; this translates to writing a createSection which can be implemented as an inlined region, look at createCritical for an example of how to do it. The clang side is also quite easy - again look at emitOMPCritical in clang for an example.

I am not going to require that you write createSection along with its clang usage to accept this patch, unless the clang lit test for omp sections doesn't work without it.

AMDChirag added inline comments.Nov 24 2020, 10:41 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1009

So, to confirm,
There should be another function called CreateSection which should be called from within CreateSections for each of the sections, and that would use the FiniCB callback?
Something like

CreateSections() {
...Init...
for each section {
    call CreateSection(SectionCB, FiniCB)
}

Where CreateSection will use the EmitOMPInlinedRegion() function?

Also, the clang lit test case for the clang side of things will be in the clang patch. I'll be creating the test.

fghanim added inline comments.Nov 26 2020, 2:09 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1009

Basically yes. to be extra specific :D
The way clang and the OMPBuilder will end up handling this

clang.emitparallel() 
  -->OMPBuilder.creatparallel( ..., bodyCB ,...) 
    --> BodyCB ()
      --> clang.emitSections() 
        --> OMPBuilder.createSections( ..., SectionCB[N], ...) 
          --> SectionCB[i]()
            --> clang.emitSection() 
              --> OMPBuilder.createsection()
                 --> {this will contain the body of section[i] and finalization code for it - essentially the same finiCB used once per section).
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2216โ€“2219

Don't forget about this. please :)

AMDChirag marked an inline comment as not done.Dec 2 2020, 10:19 PM
AMDChirag added inline comments.Dec 2 2020, 10:30 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1009

Okay, I am working on integration with EmitOMPInlinedRegion().

Regarding the flow though, I have a doubt.
With OMPIRBuilder work, the clang.EmitSections() function will not be used since it does scaffolding work which is taken care of by OMPIRBuilder.
From my understanding, the flow of functions would be something like:

Clang.EmitOMPSectionsDirective() -> OMP.CreateSections() -> (possibly) OMP.EmitOMPInlinedRegion()

where the bodygen callback will be declared in OMP.CreateSections() and SectionCBVector will be declared in Clang.EmitOMPSectionsDirective() and used in the bodygen callback.

Is that correct or am I missing something?

Thanks a bunch, btw, for helping me so much ๐Ÿ˜Š

AMDChirag marked 4 inline comments as done.Dec 22 2020, 2:15 AM
AMDChirag updated this revision to Diff 313301.EditedDec 22 2020, 5:05 AM

Updated code, test case, fixed some issues

createSections() now uses EmitOMPInlinedRegion().
The code is updated to use proper function names. Comments have been updated as well.
Test case is a lot more thorough and checks for loop variables, barrier, and loop functions.
Fixed a bug that caused crash when an external variable (outside #pragma omp section) was used.

The clang side updates will be pushed soon (D91054).

AMDChirag marked 3 inline comments as done.Dec 22 2020, 5:06 AM
AMDChirag updated this revision to Diff 313305.Dec 22 2020, 5:24 AM

Updated information for StoredBodyGenCallbackTy

One idea: Now with D90830 landed, CreateSections could be implemented by calling that (with TripCount == Number of sections), then call "CreateWorkshareLoop" (the same that we would use to implement #pragma omp for) to it, this would allow us to share the implementations of worksharing-loop and sections.

CreateWorkshareLoop does not exist yet, we might refactor this code once it exists.

What happend to using this?

This comment was removed by AMDChirag.

@jdoerfert Ouch. Totally forgot about that, apologies, on it.
Would it be fine/better to have the change for that in a separate patch though? Or must it be a part of this patch itself?

@jdoerfert Ouch. Totally forgot about that, apologies, on it.
Would it be fine/better to have the change for that in a separate patch though? Or must it be a part of this patch itself?

It should replace a lot of logic in this, right? I would prefer to use the existing functionality right away.

fghanim added inline comments.Jan 7 2021, 12:10 PM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
83โ€“86 โ†—(On Diff #313305)

This and the OMPKinds.def part: split into their own patch, and they can go separately.
LGTM for both parts.

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

Sorry for the delayed response - I got busy with a paper and a new project and this slipped through. :)

nop. Clang will always recursively call emitSection from within emitSections(), once per section. I have another diagram for you in the other comment about the recent changes.

Thanks a bunch, btw, for helping me so much ๐Ÿ˜Š

Sure, my pleasure :)

1011

This won't work as you expect it to :)

What will happen based on this code is:

clang.emitparallel() 
  -->OMPBuilder.creatparallel( ..., bodyCB ,...) 
    --> BodyCB ()
      --> clang.emitSections() 
        --> OMPBuilder.createSections( ..., SectionCB[N], ...) 
         | --> SectionCBs[i]()
         | |  --> OMPBuilderCBHelpers::EmitOMPRegionBody() --> clang.emitStmt() ---> clang.emitSection() 
         | |    --> {use clang internals to emit the section body and correct finalization}
         |  --> sectionFiniCBs[i]() //a second round of finalizations

IIRC, the second round of finalization will either assert or end up corrupting a cleanup stack inside clang (don't remember which).

As I said in the original comment on this. You don't have to get CreateSection implemented. I strongly think that the clang lit test (i.e. the test for the other patch) will work just fine without this. If it doesn't, we'll have to look into the reason why and if we need to implement createSection for it to work. -- i.e. we'll cross that bridge when we have to. :)

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2238

Nit: use OutlinedFn->getEntryBlock(). it's better.

This comment was removed by AMDChirag.
This comment was removed by AMDChirag.
AMDChirag added inline comments.Jan 9 2021, 9:02 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1011

Okay, I think (and hope) I get it now.
One question though, when using EmitOMPInlinedRegion for each section, what will the arguments for EntryCall and ExitCall parameters be? Since, as of right now, no Entry/Exit functions are used?
Must EmitOMPInlinedRegion be modified to allow nullptr for those parameters?

jdoerfert added inline comments.Jan 10 2021, 1:40 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1011

Must EmitOMPInlinedRegion be modified to allow nullptr for those parameters?

Probably. Feel free to modify the API where necessary.

Added the use of createCanonicalLoop and createStaticWorkshareLoop.
Created OMP.createSection for handling the section's code generation.
Section callbacks are no longer a part of createSections.
Modified EmitOMPInlinedRegion to be able to handle nullptr for EntryCall/ExitCall parameters.
Removed changes from OMPConstants.hpp and OMPKinds.def to, originally, move to separate differential, but now are not required since loop creation is handled with createCanonicalLoop and createStaticWorkshareLoop.
OMP.createSections no longer uses EmitOMPInlinedRegion.

AMDChirag marked 3 inline comments as done.

Address a comment.

AMDChirag marked an inline comment as done.Jan 10 2021, 10:24 PM
AMDChirag updated this revision to Diff 315740.Jan 11 2021, 2:38 AM

removed the accidentally added test statement

Currently cancellation clause is failing to work, within sections directive. Working on it.

AMDChirag updated this revision to Diff 319040.Jan 25 2021, 9:38 AM

Added IsCancellable parameter to EmitOMPInlinedRegion for sections construct.
Even after which, the cancel construct is causing an assertion failure.
For now, this is marked as an expected failure and the FIXME comment is available.
The clang test case (marked XFAIL) is available in frontend patch - D91054.

AMDChirag updated this revision to Diff 337701.Apr 15 2021, 4:08 AM

Fixed cancel codegen crash, updated test case.

AMDChirag updated this revision to Diff 337702.Apr 15 2021, 4:09 AM

Removed the mistakenly added utils file (common.py)

I only have small nits, nothing major. @fghanim, is this OK with you, if so, please accept?

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

Newline. Reference BodyGenCallBackTy for argument explanation.

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

Prefer early exits

903

No backtracking, and similar schemes, if you can just remember the blocks you are looking for when you create them.

909
1000

same comments as above, early exists & remember the blocks rather than looking for them

fghanim accepted this revision.Apr 15 2021, 7:22 PM

LGTM from me

This revision is now accepted and ready to land.Apr 15 2021, 7:22 PM
AMDChirag marked 3 inline comments as done.

Incorporated comments.

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

Could you tell me how do I remember the blocks at creation point? The blocks are not created in the same function so I believe the only way would be to pass the blocks as function arguments?
Am I missing something?

@jdoerfert Anything else to do here? Can this be merged?

AMDChirag edited the summary of this revision. (Show Details)Apr 27 2021, 11:46 PM
AMDChirag updated this revision to Diff 341435.Apr 29 2021, 1:39 AM

Rebased to HEAD of main branch

AMDChirag updated this revision to Diff 341469.Apr 29 2021, 4:25 AM

Corrected rebase

AMDChirag edited the summary of this revision. (Show Details)Apr 29 2021, 5:58 AM
AMDChirag edited the summary of this revision. (Show Details)
MaskRay added inline comments.
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2236

I fixed a -Wsign-compare here. Will be great to test the change with clang:)

AMDChirag added inline comments.Apr 29 2021, 10:36 PM
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2236

Thank you very much for the fix and the kind words :)