Page MenuHomePhabricator

[Clang][OpenMP] Frontend work for sections - D89671
ClosedPublic

Authored by AMDChirag on Nov 9 2020, 1:03 AM.

Details

Summary

This patch is child of D89671, contains the clang
implementation to use the OpenMP IRBuilder's section
construct.

Co-author: @anchu-rajendran

Diff Detail

Event Timeline

AMDChirag created this revision.Nov 9 2020, 1:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2020, 1:03 AM
AMDChirag requested review of this revision.Nov 9 2020, 1:03 AM
AMDChirag retitled this revision from [LLVM][OpenMP] Frontend work for sections - D89671 to [Clang][OpenMP] Frontend work for sections - D89671.Nov 9 2020, 1:15 AM

The test case will also be added here.

Meinersbur added inline comments.Nov 9 2020, 3:23 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
3859–3861

Instead of duplicating it here, why not using OpenMPIRBuilder::BodyGenCallbackTy?

3883

In what situation would there be no #pragma omp section children?

3891

I think this is unnecessary; SmallVector converts implicitly to an ArrayRef

fghanim added a comment.EditedNov 10 2020, 1:58 PM

Thanks for doing this.

Please add proper lit test. you can find that under clang/test/OpenMP/sections_codegen.cpp. add a case for using the OMPBuilder.
As an example look in the same directory for parallel_codegen.cpp , master_codegen.cpp , critical_codegen.cpp

In case you need it; you can check that your changes works by doing ninja/make -check-clang-openmp

Edit:
NIT: Also please change the title of the patch to be more descriptive of the contents of the patch (e.g. [clang][openmp] using OpenmpIRBuilder to implement openmp sections ) or anything you choose :)

AMDChirag updated this revision to Diff 307022.Nov 23 2020, 3:16 AM

Fixed usage of BodyGenCallbackTy
Removed ArrayRef variable

AMDChirag marked 2 inline comments as done.Nov 23 2020, 3:17 AM
AMDChirag added inline comments.Nov 23 2020, 4:33 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
3883

You are correct - I could not find a scenario where CS would be a nullptr.
However, the original implementation EmitSections() handles the CS == nullptr case as well. Also, if CS is nullptr, the software will exit with an assertion failure anyway (CodeGenFunction::EmitStmt()).
Should I just remove the else block altogether? I think an assert statement instead, for checking CS == nullptr should suffice.

AMDChirag updated this revision to Diff 313303.EditedDec 22 2020, 5:11 AM
AMDChirag marked an inline comment as done.

Updated code according to the changes in LLVM side of things.

Test cases are still required, however.

AMDChirag updated this revision to Diff 313307.Dec 22 2020, 5:27 AM

Updated BGenCallbackTy to StorableBodyGenCallbackTy.

Ping. Please add the Lit test for this.

Added OMP delegating code for createSection (EmitOMPSectionDirective).
@fghanim working on clang lit test case.

Added FIXME comment for cancellation construct not working with sections construct
Also updated the lit test cases to reflect this change.
The lit test case modification will be removed once this issue is resolved.

@fghanim @jdoerfert please review the code if/when possible.

@fghanim @jdoerfert please review the code if/when possible.

Let's resolve the issue described in the TODO before we go ahead with this. There is little point reviewing something that can't be tested.
That said,

clang/lib/CodeGen/CGStmtOpenMP.cpp
3818

Why do we unpack the children here instead of making a single call back for the CapturedStmt?

clang/test/OpenMP/cancel_sections_irbuilder.cpp
14 ↗(On Diff #319095)

I don't get it. Where is EmitOMPRegionBody called on the <xyz>.cncl block such that it would remove the terminator?

Maybe you could share the IR at the different levels if you need help resolving the problem.

AMDChirag added inline comments.Mar 15 2021, 9:49 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
3818

Because there is a bunch of additional IR (creation of switch statement and its cases/blocks, loop counter based on count of children) that is generated by createSections. If the children are not unpacked and CapturedStmt is directly passed to EmitOMPRegionBody in the callback, createSections will not be able to generate the required IR to encase the section constructs with.

AMDChirag updated this revision to Diff 337654.Apr 15 2021, 1:04 AM

Fixed cancel construct and applied updates to its test cases

The test cases are updated so that the IR generated with and without IRBuilder are considered as they are different.

The cancel codegenerator with IRBuilder causes the IR to be created in the form:

...
%29 = call i32 @__kmpc_cancel(%struct.ident_t* @1, i32 %omp_global_thread_num9, i32 3)
%30 = icmp eq i32 %29, 0
...

Whereas, without IRBuilder, the IR created is in the form:

...
%20 = call i32 @__kmpc_cancel(%struct.ident_t* @1, i32 %0, i32 3)
%21 = icmp ne i32 %20, 0
...

In essence, the output is logically same, but semantically different, hence the requirement of updating test cases.

You can update the tests as long as long as the output is correct. for example the difference is only in names, ordering of basicblocks and instructions that doesn't affect correctness, etc.

You can update the tests as long as long as the output is correct. for example the difference is only in names, ordering of basicblocks and instructions that doesn't affect correctness, etc.

Yes, the correctness is not affected with the changes to the test case.

What is the status of this? The parent D89671 has been accepted, but not committed.

AMDChirag added a comment.EditedApr 21 2021, 9:48 PM

@Meinersbur For D89671, Johannes had some comments which have been taken care of. I am just waiting for him to get back on it. I'll ping him there.
This patch is done from my end unless someone has some comment/thought.

thanks for the update

What is missing or not working in the IRBuilder impl that prevents us from turning it on by default?

What is missing or not working in the IRBuilder impl that prevents us from turning it on by default?

Per the TODO in this patch (CGStmtOpenMP.cpp - 3775), PrivatizationCallback is not working right now.
I can't think of anything else. The test cases are all working so there's that.

What is missing or not working in the IRBuilder impl that prevents us from turning it on by default?

Per the TODO in this patch (CGStmtOpenMP.cpp - 3775), PrivatizationCallback is not working right now.
I can't think of anything else. The test cases are all working so there's that.

OK, we really need to provide the PrivCB impl so we can start removing clang code.
This is generally fine with me, @fghanim @Meinersbur any concerns?

This is generally fine with me, @fghanim @Meinersbur any concerns?

I have none. All good for me

OK, we really need to provide the PrivCB impl so we can start removing clang code.
This is generally fine with me, @fghanim @Meinersbur any concerns?

It might be OK to start with the functionality that replaces directives with single runtime calls, but I don't think implementations of code-associated directives are ready enough.

jdoerfert accepted this revision.Apr 27 2021, 10:05 PM

This is generally fine with me, @fghanim @Meinersbur any concerns?

I have none. All good for me

Then LGTM.

OK, we really need to provide the PrivCB impl so we can start removing clang code.
This is generally fine with me, @fghanim @Meinersbur any concerns?

It might be OK to start with the functionality that replaces directives with single runtime calls, but I don't think implementations of code-associated directives are ready enough.

Not sure what "not ready enough" means. If the functionality is equivalent we can switch over and remove duplication, if not, we can't.
Take omp master, which has associated code, but for which I'm not aware of a reason why we can't switch over. We should double check and if we can't find a problem we do it.
There is no point in waiting if we don't look at / work on the stuff.

This revision is now accepted and ready to land.Apr 27 2021, 10:05 PM
AMDChirag edited the summary of this revision. (Show Details)Apr 28 2021, 10:24 PM
AMDChirag updated this revision to Diff 341436.EditedApr 29 2021, 1:42 AM

Rebased to HEAD of main branch

AMDChirag edited the summary of this revision. (Show Details)Apr 29 2021, 6:37 AM
AMDChirag updated this revision to Diff 341503.Apr 29 2021, 6:43 AM

Rebase to retrigger build

The build failed previously - seemingly with a test case that has nothing to do with this patch.
Retriggering and Rebasing the commit to ensure the same.

This revision was automatically updated to reflect the committed changes.