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

[AMDChirag]

Summary

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

Co-author: @anchu-rajendran

[AMDChirag]

The test case will also be added here.

[Meinersbur]

clang/lib/CodeGen/CGStmtOpenMP.cpp

3552–3554

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

3576

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

3584

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

[fghanim]

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]

Fixed usage of BodyGenCallbackTy
Removed ArrayRef variable

[AMDChirag]

clang/lib/CodeGen/CGStmtOpenMP.cpp

3576

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 code according to the changes in LLVM side of things.

Test cases are still required, however.

[AMDChirag]

Updated BGenCallbackTy to StorableBodyGenCallbackTy.

[fghanim]

Ping. Please add the Lit test for this.

[AMDChirag]

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

[AMDChirag]

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.

[AMDChirag]

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

[jdoerfert]

In D91054#2548266, @AMDChirag wrote:

@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

3573

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]

clang/lib/CodeGen/CGStmtOpenMP.cpp

3573

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]

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.

[fghanim]

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.

[AMDChirag]

In D91054#2693514, @fghanim wrote:

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.

[Meinersbur]

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

[AMDChirag]

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

[Meinersbur]

thanks for the update

[jdoerfert]

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

[AMDChirag]

In D91054#2708873, @jdoerfert wrote:

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.

[jdoerfert]

In D91054#2708924, @AMDChirag wrote:

In D91054#2708873, @jdoerfert wrote:

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?

[fghanim]

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

I have none. All good for me

[Meinersbur]

In D91054#2709712, @jdoerfert wrote:

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]

In D91054#2711654, @fghanim wrote:

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

I have none. All good for me

Then LGTM.

In D91054#2721194, @Meinersbur wrote:

In D91054#2709712, @jdoerfert wrote:

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.

[AMDChirag]

Rebased to HEAD of main branch

[AMDChirag]

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.

clang/lib/CodeGen/CGStmtOpenMP.cpp

if (HasLastprivates && S.getSingleClause<OMPNowaitClause>()) {
     // Emit implicit barrier to synchronize threads and avoid data races on
     // initialization of firstprivate variables.
     CGM.getOpenMPRuntime().emitBarrierCall(*this, S.getBeginLoc(),
                                            OMPD_unknown);
   }
 }
 
 void CodeGenFunction::EmitOMPSectionsDirective(const OMPSectionsDirective &S) {
-  {
-    auto LPCRegion =
-        CGOpenMPRuntime::LastprivateConditionalRAII::disable(*this, S);
-    OMPLexicalScope Scope(*this, S, OMPD_unknown);
-    EmitSections(S);
-  }
+  if (CGM.getLangOpts().OpenMPIRBuilder) {
+    llvm::OpenMPIRBuilder &OMPBuilder = CGM.getOpenMPRuntime().getOMPBuilder();
+    using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
+    using BodyGenCallbackTy =
+        llvm::function_ref<void(InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+                                llvm::BasicBlock & ContinuationBB)>;
+
+    auto FiniCB = [this](InsertPointTy IP) {
+      OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
+    };
+
+    const Stmt *CapturedStmt = S.getInnermostCapturedStmt()->getCapturedStmt();
+    const auto *CS = dyn_cast<CompoundStmt>(CapturedStmt);
+    llvm::SmallVector<BodyGenCallbackTy, 4> SectionCBVector;
+    if (CS) {
+      for (const Stmt *SubStmt : CS->children()) {
+        auto SectionCB = [this, SubStmt](InsertPointTy AllocaIP,
+                                         InsertPointTy CodeGenIP,
+                                         llvm::BasicBlock &FiniBB) {
+          OMPBuilderCBHelpers::InlinedRegionBodyRAII IRB(*this, AllocaIP,
+                                                         FiniBB);
+          OMPBuilderCBHelpers::EmitOMPRegionBody(*this, SubStmt, CodeGenIP,
+                                                 FiniBB);
+        };
+        SectionCBVector.push_back(SectionCB);
+      }
+    } else {
+      auto SectionCB = [this, CS](InsertPointTy AllocaIP,
+                                  InsertPointTy CodeGenIP,
+                                  llvm::BasicBlock &FiniBB) {
+        OMPBuilderCBHelpers::InlinedRegionBodyRAII IRB(*this, AllocaIP, FiniBB);
+        OMPBuilderCBHelpers::EmitOMPRegionBody(*this, CS, CodeGenIP, FiniBB);
+      };
+      SectionCBVector.push_back(SectionCB);
+    }
+    ArrayRef<BodyGenCallbackTy> SectionCBs = makeArrayRef(SectionCBVector);
+
+    // Privatization callback that performs appropriate action for
+    // shared/private/firstprivate/lastprivate/copyin/... variables.
+    //
+    // TODO: This defaults to shared right now.
+    auto PrivCB = [](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+                     llvm::Value &Val, llvm::Value *&ReplVal) {
+      // The next line is appropriate only for variables (Val) with the
+      // data-sharing attribute "shared".
+      ReplVal = &Val;
+
+      return CodeGenIP;
+    };
+
+    llvm::OpenMPIRBuilder::InsertPointTy AllocaIP(
+        AllocaInsertPt->getParent(), AllocaInsertPt->getIterator());
+    Builder.restoreIP(OMPBuilder.CreateSections(
+        Builder, AllocaIP, SectionCBs, PrivCB, FiniCB, S.hasCancel(),
+        S.getSingleClause<OMPNowaitClause>()));
+
+    return;
+  }
   // Emit an implicit barrier at the end.
   if (!S.getSingleClause<OMPNowaitClause>()) {
     CGM.getOpenMPRuntime().emitBarrierCall(*this, S.getBeginLoc(),
                                            OMPD_sections);
   }
   // Check for outer lastprivate conditional update.
   checkForLastprivateConditionalUpdate(*this, S);
 }