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

[AMDChirag]

Summary

Primary Author: @anchu-rajendran

This patch adds section support in the OpenMP IRBuilder module. Also added a test for it.

[AMDChirag]

This comment was removed by AMDChirag.

[AMDChirag]

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

[jdoerfert]

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.

[fghanim]

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.

[AMDChirag]

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

[jdoerfert]

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]

squashed the commits

[AMDChirag]

Minor corrections in naming

[AMDChirag]

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]

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(...);

[Meinersbur]

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.

[fghanim]

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]

Added FiniCB usage and Control Flow Graph

[fghanim]

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]

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]

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]

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]

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]

Updated information for StoredBodyGenCallbackTy

[jdoerfert]

In D89671#2383849, @Meinersbur wrote:

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?

[AMDChirag]

This comment was removed by AMDChirag.

[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]

In D89671#2484859, @AMDChirag wrote:

@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]

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.

[AMDChirag]

This comment was removed by AMDChirag.

[AMDChirag]

This comment was removed by AMDChirag.

[AMDChirag]

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]

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.

[AMDChirag]

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]

Address a comment.

[AMDChirag]

removed the accidentally added test statement

[AMDChirag]

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