[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder
Meinersbur added a comment.
While manually editing `ordered_codegen.cpp` gives nicer results, re-running
`update_cc_test_checks.py` would be less work. Your manual changes would be
also be dropped the next time somebody runs update_cc_test_checks.py and
commits.
The code seems derived from @fghanim single/master/etc implementation, would be
nice if they could have a look.
The non-OMPBuilder code emits an outlined `__captured_stmt`, but not with
OMPBuilder enabled. I assume this is due to the missing `finatlize` call.
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5335
+ llvm::BasicBlock ) {
+// TODO: The ordered directive with simd clause.
+
Also, out and `assert`/`llvm_unreachable` here?
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5323-5325
+Range = AssertSuccess(Actions.BuildBinOp(
+nullptr, {}, BO_Add, Range,
+Actions.ActOnIntegerConstant(SourceLocation(), 1).get()));
Haven't really understood why this is necessary, but this new version LGTM.
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2154
+ Builder.restoreIP(
+ OMPBuilder.createOrdered(Builder, BodyGenCB, FiniCB, false));
+
Did you intend to pass IsThreads=true. Currently it is failing because no
`__kmpc_ordered` is emitted.
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2156-2157
+
+ Value *EntryBBTI = EntryBB->getTerminator();
+ EXPECT_EQ(EntryBBTI, nullptr);
+
Consider emitting a terminator, call `finalize()` and `verifyModule`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107430/new/
https://reviews.llvm.org/D107430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder
peixin updated this revision to Diff 365170.
peixin added a comment.
Revise the implementation by not generating "kmp_ordered" runtime functions
when -fopenmp-enable-irbuilder is not enabled, which is consistent with that
not using OpenMPIRBuilder.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107430/new/
https://reviews.llvm.org/D107430
Files:
clang/lib/CodeGen/CGStmtOpenMP.cpp
clang/lib/Sema/SemaOpenMP.cpp
clang/test/OpenMP/ordered_codegen.cpp
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -2115,6 +2115,78 @@
EXPECT_EQ(CriticalEndCI->getArgOperand(2)->getType(), CriticalNamePtrTy);
}
+TEST_F(OpenMPIRBuilderTest, OrderedDirective) {
+ using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+ OpenMPIRBuilder OMPBuilder(*M);
+ OMPBuilder.initialize();
+ F->setName("func");
+ IRBuilder<> Builder(BB);
+
+ OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+
+ AllocaInst *PrivAI =
+ Builder.CreateAlloca(F->arg_begin()->getType(), nullptr, "priv.inst");
+
+ BasicBlock *EntryBB = nullptr;
+
+ auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+ BasicBlock ) {
+EntryBB = FiniBB.getUniquePredecessor();
+
+llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+llvm::Instruction *CodeGenIPInst = &*CodeGenIP.getPoint();
+EXPECT_EQ(CodeGenIPBB->getTerminator(), CodeGenIPInst);
+EXPECT_EQ(EntryBB, CodeGenIPBB);
+
+Builder.restoreIP(CodeGenIP);
+Builder.CreateStore(F->arg_begin(), PrivAI);
+Value *PrivLoad =
+Builder.CreateLoad(PrivAI->getAllocatedType(), PrivAI, "local.use");
+Builder.CreateICmpNE(F->arg_begin(), PrivLoad);
+ };
+
+ auto FiniCB = [&](InsertPointTy IP) {
+BasicBlock *IPBB = IP.getBlock();
+EXPECT_NE(IPBB->end(), IP.getPoint());
+ };
+
+ Builder.restoreIP(
+ OMPBuilder.createOrdered(Builder, BodyGenCB, FiniCB, false));
+
+ Value *EntryBBTI = EntryBB->getTerminator();
+ EXPECT_EQ(EntryBBTI, nullptr);
+
+ CallInst *OrderedEntryCI = nullptr;
+ for (auto : *EntryBB) {
+Instruction *Cur =
+if (isa(Cur)) {
+ OrderedEntryCI = cast(Cur);
+ if (OrderedEntryCI->getCalledFunction()->getName() == "__kmpc_ordered")
+break;
+ OrderedEntryCI = nullptr;
+}
+ }
+ EXPECT_NE(OrderedEntryCI, nullptr);
+ EXPECT_EQ(OrderedEntryCI->getNumArgOperands(), 2U);
+ EXPECT_EQ(OrderedEntryCI->getCalledFunction()->getName(), "__kmpc_ordered");
+ EXPECT_TRUE(isa(OrderedEntryCI->getArgOperand(0)));
+
+ CallInst *OrderedEndCI = nullptr;
+ for (auto : *EntryBB) {
+Instruction *Cur =
+if (isa(Cur)) {
+ OrderedEndCI = cast(Cur);
+ if (OrderedEndCI->getCalledFunction()->getName() == "__kmpc_end_ordered")
+break;
+ OrderedEndCI = nullptr;
+}
+ }
+ EXPECT_NE(OrderedEndCI, nullptr);
+ EXPECT_EQ(OrderedEndCI->getNumArgOperands(), 2U);
+ EXPECT_TRUE(isa(OrderedEndCI->getArgOperand(0)));
+ EXPECT_EQ(OrderedEndCI->getArgOperand(1), OrderedEntryCI->getArgOperand(1));
+}
+
TEST_F(OpenMPIRBuilderTest, CopyinBlocks) {
OpenMPIRBuilder OMPBuilder(*M);
OMPBuilder.initialize();
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -2133,6 +2133,35 @@
/*Conditional*/ false, /*hasFinalize*/ true);
}
+OpenMPIRBuilder::InsertPointTy
+OpenMPIRBuilder::createOrdered(const LocationDescription ,
+ BodyGenCallbackTy BodyGenCB,
+ FinalizeCallbackTy FiniCB, bool IsThreads) {
+ if (!updateToLocation(Loc))
+return Loc.IP;
+
+ Directive OMPD = Directive::OMPD_ordered;
+ Instruction *EntryCall = nullptr;
+ Instruction *ExitCall = nullptr;
+
+ if (IsThreads) {
+Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
+Value *Ident = getOrCreateIdent(SrcLocStr);
+Value *ThreadId = getOrCreateThreadID(Ident);
+Value *Args[] = {Ident, ThreadId};
+
+Function *EntryRTLFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_ordered);
+EntryCall = Builder.CreateCall(EntryRTLFn, Args);
+
+Function *ExitRTLFn =
+getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_end_ordered);
+ExitCall = Builder.CreateCall(ExitRTLFn, Args);
+ }
+
+ return EmitOMPInlinedRegion(OMPD, EntryCall, ExitCall, BodyGenCB, FiniCB,
+ /*Conditional*/ false, /*hasFinalize*/ true);
+}
+
OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::EmitOMPInlinedRegion(
Directive OMPD, Instruction *EntryCall, Instruction *ExitCall,
[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder
peixin updated this revision to Diff 364389.
peixin added a comment.
@Meinersbur Thanks for the review.
Removed the typo fixes and gave the alloca register one name.
Also found some typos of invalid case style for variable 'cur' in
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp and will fix them when pushing
this patch to main branch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107430/new/
https://reviews.llvm.org/D107430
Files:
clang/lib/CodeGen/CGStmtOpenMP.cpp
clang/lib/Sema/SemaOpenMP.cpp
clang/test/OpenMP/ordered_codegen.cpp
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -2113,6 +2113,77 @@
EXPECT_EQ(CriticalEndCI->getArgOperand(2)->getType(), CriticalNamePtrTy);
}
+TEST_F(OpenMPIRBuilderTest, OrderedDirective) {
+ using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+ OpenMPIRBuilder OMPBuilder(*M);
+ OMPBuilder.initialize();
+ F->setName("func");
+ IRBuilder<> Builder(BB);
+
+ OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+
+ AllocaInst *PrivAI =
+ Builder.CreateAlloca(F->arg_begin()->getType(), nullptr, "priv.inst");
+
+ BasicBlock *EntryBB = nullptr;
+
+ auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+ BasicBlock ) {
+EntryBB = FiniBB.getUniquePredecessor();
+
+llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+llvm::Instruction *CodeGenIPInst = &*CodeGenIP.getPoint();
+EXPECT_EQ(CodeGenIPBB->getTerminator(), CodeGenIPInst);
+EXPECT_EQ(EntryBB, CodeGenIPBB);
+
+Builder.restoreIP(CodeGenIP);
+Builder.CreateStore(F->arg_begin(), PrivAI);
+Value *PrivLoad =
+Builder.CreateLoad(PrivAI->getAllocatedType(), PrivAI, "local.use");
+Builder.CreateICmpNE(F->arg_begin(), PrivLoad);
+ };
+
+ auto FiniCB = [&](InsertPointTy IP) {
+BasicBlock *IPBB = IP.getBlock();
+EXPECT_NE(IPBB->end(), IP.getPoint());
+ };
+
+ Builder.restoreIP(OMPBuilder.createOrdered(Builder, BodyGenCB, FiniCB));
+
+ Value *EntryBBTI = EntryBB->getTerminator();
+ EXPECT_EQ(EntryBBTI, nullptr);
+
+ CallInst *OrderedEntryCI = nullptr;
+ for (auto : *EntryBB) {
+Instruction *Cur =
+if (isa(Cur)) {
+ OrderedEntryCI = cast(Cur);
+ if (OrderedEntryCI->getCalledFunction()->getName() == "__kmpc_ordered")
+break;
+ OrderedEntryCI = nullptr;
+}
+ }
+ EXPECT_NE(OrderedEntryCI, nullptr);
+ EXPECT_EQ(OrderedEntryCI->getNumArgOperands(), 2U);
+ EXPECT_EQ(OrderedEntryCI->getCalledFunction()->getName(), "__kmpc_ordered");
+ EXPECT_TRUE(isa(OrderedEntryCI->getArgOperand(0)));
+
+ CallInst *OrderedEndCI = nullptr;
+ for (auto : *EntryBB) {
+Instruction *Cur =
+if (isa(Cur)) {
+ OrderedEndCI = cast(Cur);
+ if (OrderedEndCI->getCalledFunction()->getName() == "__kmpc_end_ordered")
+break;
+ OrderedEndCI = nullptr;
+}
+ }
+ EXPECT_NE(OrderedEndCI, nullptr);
+ EXPECT_EQ(OrderedEndCI->getNumArgOperands(), 2U);
+ EXPECT_TRUE(isa(OrderedEndCI->getArgOperand(0)));
+ EXPECT_EQ(OrderedEndCI->getArgOperand(1), OrderedEntryCI->getArgOperand(1));
+}
+
TEST_F(OpenMPIRBuilderTest, CopyinBlocks) {
OpenMPIRBuilder OMPBuilder(*M);
OMPBuilder.initialize();
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -1960,6 +1960,30 @@
/*Conditional*/ false, /*hasFinalize*/ true);
}
+OpenMPIRBuilder::InsertPointTy
+OpenMPIRBuilder::createOrdered(const LocationDescription ,
+ BodyGenCallbackTy BodyGenCB,
+ FinalizeCallbackTy FiniCB) {
+ if (!updateToLocation(Loc))
+return Loc.IP;
+
+ Directive OMPD = Directive::OMPD_ordered;
+ Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
+ Value *Ident = getOrCreateIdent(SrcLocStr);
+ Value *ThreadId = getOrCreateThreadID(Ident);
+ Value *Args[] = {Ident, ThreadId};
+
+ Function *EntryRTLFn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_ordered);
+ Instruction *EntryCall = Builder.CreateCall(EntryRTLFn, Args);
+
+ Function *ExitRTLFn =
+ getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_end_ordered);
+ Instruction *ExitCall = Builder.CreateCall(ExitRTLFn, Args);
+
+ return EmitOMPInlinedRegion(OMPD, EntryCall, ExitCall, BodyGenCB, FiniCB,
+ /*Conditional*/ false, /*hasFinalize*/ true);
+}
+
OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::EmitOMPInlinedRegion(
Directive OMPD, Instruction *EntryCall, Instruction *ExitCall,
BodyGenCallbackTy BodyGenCB,
[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder
Meinersbur added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:708 /// - /// \returns The insertion position *after* the master. + /// \returns The insertion position *after* the masked. InsertPointTy createMasked(const LocationDescription , Could you commit these typo fixes separately? That is, just push it to main, no Phabricator review required. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:721 /// - /// \returns The insertion position *after* the master. + /// \returns The insertion position *after* the critical. InsertPointTy createCritical(const LocationDescription , Could you commit these typo fixes separately? That is, just push it to main, no Phabricator review required. Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2125 + + AllocaInst *PrivAI = Builder.CreateAlloca(F->arg_begin()->getType()); + Could you give this alloca register a name? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107430/new/ https://reviews.llvm.org/D107430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder
peixin added a comment. @kiranchandramohan Thanks for the review. Removed the OpenMP 1.0 target from the description. The simd clause does need more changes and I think it also depend simd directive in the OpenMPIRBuilder. According to the CHECK5 in the test case `ordered_codegen.cpp`, it does not create `kmp_ordered` runtime functions call with the simd clause. I have not investigated too much about the standalone ordered construct. To be honest, I am not sure if supporting simd and depend clauses in CreateOrdered is better or not. My current plan is to look into the Flang MLIR Op Def and LLVM IR to understand how the IRBuilder is used. After finishing the end-to-end implementation of ordered construct without simd and depend clauses, I will come back to try to implement the IRBuilder of ordered directive with simd and depend clauses. At that time, Arnamoy should also have time to implement the IRBuilder for simd directive. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107430/new/ https://reviews.llvm.org/D107430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder
kiranchandramohan added a comment. Thanks @peixin for the patch. Could you remove the OpenMP 1.0 target from the description? That is a target for the Flang team and since this patch touches clang as well, it might be confusing. I believe this is modelling the ordered construct with a region. Could simd be another option to CreateOrdered or does it need more changes? Also, could you clarify how the standalone ordered construct (with depend) will be handled? Will it be an extension to CreateOrdered or will be a completely separate function? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107430/new/ https://reviews.llvm.org/D107430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
