This is an archive of the discontinued LLVM Phabricator instance.

[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

AMDChirag created this revision.Oct 19 2020, 12:39 AM
AMDChirag requested review of this revision.Oct 19 2020, 12:39 AM
fghanim added a subscriber: fghanim.

Thank you for working on this!

Could you post the review for the clang usage of this. I think it is important that this is included so we can understand how this is going to be used. Please add it as a child revision to this one so it is not lost in the comments.

Also, Please change the title from [flang][openmp].... to [llvm][openmp]....; While the biggest beneficiary from the OMPBuilder is flang, it is part of the LLVM project, not flang. Also, this way the proper filters are triggered for people who are interested in this. :)

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
48–50

This is a duplicate of BodyGenCallbackTy. Please remove and replace uses with that instead.

llvm/include/llvm/IR/IRBuilder.h
1652–1658 ↗(On Diff #298942)

Few points:
1- Do you use this outside the OMPBuilder? if no, then please move it there as a private method
2- NIT: it is preferable that this goes in separate patch - if we decide to keep it here

1655 ↗(On Diff #298942)

Is LVal going to always be constant? if yes, then please add checks to make sure of that.
If not, then I believe it is better to not have this entire creator to begin with. since Alloca needs to go into entry block, and non-constant LVal may be created at a later BB.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
852–859

All allocaInst created here need to go into the entry block of the function (or in this case, the OMP outlined region). Please insert these into the entry block.

Until we figure out where to keep allocaIP, pass it from the Frontend as a parameter to createSections. Clang is properly set up to always have the correct one at all times, and I have a strong suspicion that we can do the same on the OMP-IR side of things.

865–871

A little diagram comment for the structure of the intended CFG is highly appreciated and super helpful. :)

866–871

Here and elsewhere, for BasicBlock::Create() - insertAfter combos : i think the insertafter is redundant. You can specify the function a basic block (BB) belongs to as part of its creation, and if a BB to insert before is not specified, it will be automatically added as last block in list.
if you expect there may be a BB after InsertBB, then I think it's worthwhile to get that block and use that as the insertBefore BB.

FWIW, ordering in the blocklist is not super important, so you may just add it wherever you want

918

Just wanna make sure I am not missing something; Are using this callback anywhere other than here? if not, Why not just inline its contents in place of this callback?

AMDChirag retitled this revision from [Flang][OpenMP] Adding support for OpenMP sections construct in OpenMPIRBuilder to [LLVM][OpenMP] Adding support for OpenMP sections construct in OpenMPIRBuilder.Oct 19 2020, 8:47 PM

@fghanim Hello and thank you!

I have updated the title.

Apologies, I am not well versed with this, but could you explain what does LVal being always constant mean in this context?
And also, what sort of checks do I add for that?

I have updated the title.

Great .. Thanks!

Apologies, I am not well versed with this, but could you explain what does LVal being always constant mean in this context?

No worries. we are all at different stages of learning.
Welcome to LLVM. :)

I saw that you always use createLVal with a contant value for initialization. So my Questions was are we always going to initialize the LVal we created with a constant value or not? because if not, we have the problem I mentioned in the original comment.

And also, what sort of checks do I add for that?

a simple assert with a "nice descriptive message" or any other more appropriate check you want is good enough;

This comment was removed by AMDChirag.
SouraVX added a project: Restricted Project.Oct 21 2020, 1:17 AM
AMDChirag updated this revision to Diff 299594.Oct 21 2020, 1:39 AM
This comment was removed by AMDChirag.
AMDChirag updated this revision to Diff 299597.Oct 21 2020, 1:46 AM
AMDChirag marked 2 inline comments as done.

Amended the original commit, applying the fixes:

  • removed BGenCallbackTy (was a duplicate of BodyGenCallbackTy)
  • removed CreateLVal function
  • all allocas for sections internal variables are grouped together
  • removed InsertAfter() calls since they were redundant
  • inlined the CreateSwitchCB callback

Some drive-by comments. I will assume @fghanim continues the review, feel free to not wait on me.

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

This should go into OMPKinds.def or the tablegen file.

502

Commit things like this without review as NFC changes.

523

Is nowait has no doxygen comment above. I would prefer IsNowait as spelling.

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

Nit: CurIP (to match the other spelling above).

933

Comments, here and elsewhere, should be proper sentences.

960

No braces.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1471

FWIW, tt never will be, that is a front-end task.

Great work. Thanks.

Also, please add the revision where this is used in clang as a child revision. It's a great validity check for this, and also showcases the intended way this is expected to be used. Up to this point I have been guessing the intent.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
26–54

Thanks for moving these over.

Where do you use them in this patch? I cannot find the usage.

In any case, and as pointed out by @jdoerfert , these belong in OpenMPKInds.def, not here. But in all likelyhood we already have them there. Please verify whether they already exist or not, and if they do use what we already have.
if not then move them over and use a similar style (look for OMP_IDENT_FLAG as an example to see what I mean).
If you don't use them here, then please do so in a separate patch - I suspect it may turn out to be a bit involved.

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

Where do you call the FiniCB callback? I cannot find it anywhere.

if not, then you probably should. this is where finalization code is done (e.g. destructors call + any other cleanup code)

At this stage, I am not sure if we need a single Finicb for the entire thing, or one per SectionCB.

873–878

Maybe you missed this. A little diagram for the intended CFG structure as a comment would be very helpful.

888

NIT: Give the CmpRef instruction a name more descriptive than "cmp" - It will make reading the resultant IR much easier :)

898–909

regarding my the request for a CFG diagram ; You can add to the comment above -in similar style- to show how the BBs created for the loop interact with this/ look like.

916

I think from the perspective of C/C++, each one of these cases is a scope that should have a finalization block that we use for cleanup code. as such I am not sure if having one block for all cleanup is the right thing to do. This is why I am asking about the revision for usage in clang.

951

Do you use this callback anywhere else? if not, inline this as well.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1488–1491

I don't think these checks are adequate. Generally, what you should be testing for is the generator's "defining features" - for lack of better name. examples for things you should be testing

  • your loop - do we have the proper bounds, does it have the correct checks, is it generating the correct branches to name a few.
  • are we generating correct sections, and are they terminated appropriately (i.e. do they branch to correct/corresponding FiniCB) etc.
  • is the switchInst being created correct (correct number of cases, branches to correct blocks for each case, etc.)
  • all omp runtime calls (are we generating all correct runtime calls, correct arg., correct locations, etc.)
  • any other thing you think is important and needs to be regularly tested
1494

OK, so you are not calling FiniCB anywhere. This is wrong. please modify the implementation to call the proper FiniCB, and change this test to reflect that.

sameeranjoshi resigned from this revision.Oct 29 2020, 10:38 PM
AMDChirag updated this revision to Diff 302248.EditedNov 2 2020, 5:23 AM

Applied some review changes

Moved the scheduler type enum to OpenMPKinds.def, inlined
CreateForLoopCB callback.
Some review changes are still being worked on.
The clang side of change will have a new revision created soon as the
child of this revision (D89671).

The diff is against the previous version, not against a base version in LLVM. I usually squash all changes into a single commit but there are other ways too.

AMDChirag updated this revision to Diff 302500.Nov 3 2020, 12:56 AM

squashed the commits

AMDChirag updated this revision to Diff 303122.Nov 5 2020, 7:47 AM
AMDChirag marked 10 inline comments as done.

Minor corrections in naming

AMDChirag added inline comments.Nov 9 2020, 1:33 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
841

Hi @fghanim
Could you please tell me how the multiple FiniCB thing will work? Will there be separate definitions for the FiniCBs or the single definition (which is right now) will be used multiple times (= the number of section CBs)?
The frontend work has been added - revision D91054.

AMDChirag marked an inline comment as not done.Nov 9 2020, 1:36 AM
AMDChirag added inline comments.
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
916

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
841

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

916

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
523

Nit: add Doxygen documentation for IsNoWait

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

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

966

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
966

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
966

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
1488–1491

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
966

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
966

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 :)

968

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
1510

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
968

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
968

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
100

Newline. Reference BodyGenCallBackTy for argument explanation.

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

Prefer early exits

860

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

866
957

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
860

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
1508

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
1508

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