Skip to content

Commit ad9b4eb

Browse files
luporlluporl
committed
Address review's comments
1 parent 75fde1a commit ad9b4eb

File tree

4 files changed

+72
-42
lines changed

4 files changed

+72
-42
lines changed

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1829,15 +1829,13 @@ class OpenMPIRBuilder {
18291829
/// \param BodyGenCB Callback that will generate the region code.
18301830
/// \param FiniCB Callback to finalize variable copies.
18311831
/// \param IsNowait If false, a barrier is emitted.
1832-
/// \param DidIt Local variable used as a flag to indicate 'single' thread
18331832
/// \param CPVars copyprivate variables.
18341833
/// \param CPFuncs copy functions to use for each copyprivate variable.
18351834
///
18361835
/// \returns The insertion position *after* the single call.
18371836
InsertPointTy createSingle(const LocationDescription &Loc,
18381837
BodyGenCallbackTy BodyGenCB,
18391838
FinalizeCallbackTy FiniCB, bool IsNowait,
1840-
llvm::Value *DidIt,
18411839
ArrayRef<llvm::Value *> CPVars = {},
18421840
ArrayRef<llvm::Function *> CPFuncs = {});
18431841

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4002,14 +4002,16 @@ OpenMPIRBuilder::createCopyPrivate(const LocationDescription &Loc,
40024002

40034003
OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createSingle(
40044004
const LocationDescription &Loc, BodyGenCallbackTy BodyGenCB,
4005-
FinalizeCallbackTy FiniCB, bool IsNowait, llvm::Value *DidIt,
4006-
ArrayRef<llvm::Value *> CPVars, ArrayRef<llvm::Function *> CPFuncs) {
4005+
FinalizeCallbackTy FiniCB, bool IsNowait, ArrayRef<llvm::Value *> CPVars,
4006+
ArrayRef<llvm::Function *> CPFuncs) {
40074007

40084008
if (!updateToLocation(Loc))
40094009
return Loc.IP;
40104010

4011-
// If needed (i.e. not null), initialize `DidIt` with 0
4012-
if (DidIt) {
4011+
// If needed allocate and initialize `DidIt` with 0
4012+
llvm::Value *DidIt = nullptr;
4013+
if (!CPVars.empty()) {
4014+
DidIt = Builder.CreateAlloca(llvm::Type::getInt32Ty(Builder.getContext()));
40134015
Builder.CreateStore(Builder.getInt32(0), DidIt);
40144016
}
40154017

‎llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Lines changed: 65 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -3327,8 +3327,8 @@ TEST_F(OpenMPIRBuilderTest, SingleDirective) {
33273327
EXPECT_NE(IPBB->end(), IP.getPoint());
33283328
};
33293329

3330-
Builder.restoreIP(OMPBuilder.createSingle(
3331-
Builder, BodyGenCB, FiniCB, /*IsNowait*/ false, /*DidIt*/ nullptr));
3330+
Builder.restoreIP(
3331+
OMPBuilder.createSingle(Builder, BodyGenCB, FiniCB, /*IsNowait*/ false));
33323332
Value *EntryBBTI = EntryBB->getTerminator();
33333333
EXPECT_NE(EntryBBTI, nullptr);
33343334
EXPECT_TRUE(isa<BranchInst>(EntryBBTI));
@@ -3417,8 +3417,8 @@ TEST_F(OpenMPIRBuilderTest, SingleDirectiveNowait) {
34173417
EXPECT_NE(IPBB->end(), IP.getPoint());
34183418
};
34193419

3420-
Builder.restoreIP(OMPBuilder.createSingle(
3421-
Builder, BodyGenCB, FiniCB, /*IsNowait*/ true, /*DidIt*/ nullptr));
3420+
Builder.restoreIP(
3421+
OMPBuilder.createSingle(Builder, BodyGenCB, FiniCB, /*IsNowait*/ true));
34223422
Value *EntryBBTI = EntryBB->getTerminator();
34233423
EXPECT_NE(EntryBBTI, nullptr);
34243424
EXPECT_TRUE(isa<BranchInst>(EntryBBTI));
@@ -3464,6 +3464,26 @@ TEST_F(OpenMPIRBuilderTest, SingleDirectiveNowait) {
34643464
EXPECT_EQ(ExitBarrier, nullptr);
34653465
}
34663466

3467+
// Helper class to check each instruction of a BB.
3468+
class BBInstIter {
3469+
BasicBlock *BB;
3470+
BasicBlock::iterator BBI;
3471+
3472+
public:
3473+
BBInstIter(BasicBlock *BB) : BB(BB), BBI(BB->begin()) {}
3474+
3475+
bool hasNext() const { return BBI != BB->end(); }
3476+
3477+
template <typename InstTy> InstTy *next() {
3478+
if (!hasNext())
3479+
return nullptr;
3480+
Instruction *Cur = &*BBI++;
3481+
if (!isa<InstTy>(Cur))
3482+
return nullptr;
3483+
return cast<InstTy>(Cur);
3484+
}
3485+
};
3486+
34673487
TEST_F(OpenMPIRBuilderTest, SingleDirectiveCopyPrivate) {
34683488
using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
34693489
OpenMPIRBuilder OMPBuilder(*M);
@@ -3486,8 +3506,6 @@ TEST_F(OpenMPIRBuilderTest, SingleDirectiveCopyPrivate) {
34863506
Function *CopyFunc =
34873507
Function::Create(CopyFuncTy, Function::PrivateLinkage, "copy_var", *M);
34883508

3489-
Value *DidIt = Builder.CreateAlloca(Type::getInt32Ty(Builder.getContext()));
3490-
34913509
auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP) {
34923510
if (AllocaIP.isSet())
34933511
Builder.restoreIP(AllocaIP);
@@ -3514,11 +3532,12 @@ TEST_F(OpenMPIRBuilderTest, SingleDirectiveCopyPrivate) {
35143532

35153533
auto FiniCB = [&](InsertPointTy IP) {
35163534
BasicBlock *IPBB = IP.getBlock();
3535+
// IP must be before the unconditional branch to ExitBB
35173536
EXPECT_NE(IPBB->end(), IP.getPoint());
35183537
};
35193538

35203539
Builder.restoreIP(OMPBuilder.createSingle(Builder, BodyGenCB, FiniCB,
3521-
/*IsNowait*/ false, DidIt, {CPVar},
3540+
/*IsNowait*/ false, {CPVar},
35223541
{CopyFunc}));
35233542
Value *EntryBBTI = EntryBB->getTerminator();
35243543
EXPECT_NE(EntryBBTI, nullptr);
@@ -3537,42 +3556,57 @@ TEST_F(OpenMPIRBuilderTest, SingleDirectiveCopyPrivate) {
35373556
EXPECT_EQ(SingleEntryCI->getCalledFunction()->getName(), "__kmpc_single");
35383557
EXPECT_TRUE(isa<GlobalVariable>(SingleEntryCI->getArgOperand(0)));
35393558

3540-
CallInst *SingleEndCI = nullptr;
3541-
for (auto &FI : *ThenBB) {
3542-
Instruction *Cur = &FI;
3543-
if (isa<CallInst>(Cur)) {
3544-
SingleEndCI = cast<CallInst>(Cur);
3545-
if (SingleEndCI->getCalledFunction()->getName() == "__kmpc_end_single")
3546-
break;
3547-
SingleEndCI = nullptr;
3548-
}
3549-
}
3559+
// check ThenBB
3560+
BBInstIter ThenBBI(ThenBB);
3561+
// load PrivAI
3562+
auto *PrivLI = ThenBBI.next<LoadInst>();
3563+
EXPECT_NE(PrivLI, nullptr);
3564+
EXPECT_EQ(PrivLI->getPointerOperand(), PrivAI);
3565+
// icmp
3566+
EXPECT_TRUE(ThenBBI.next<ICmpInst>());
3567+
// store 1, DidIt
3568+
auto *DidItSI = ThenBBI.next<StoreInst>();
3569+
EXPECT_NE(DidItSI, nullptr);
3570+
EXPECT_EQ(DidItSI->getValueOperand(),
3571+
ConstantInt::get(Type::getInt32Ty(Ctx), 1));
3572+
Value *DidIt = DidItSI->getPointerOperand();
3573+
// call __kmpc_end_single
3574+
auto *SingleEndCI = ThenBBI.next<CallInst>();
35503575
EXPECT_NE(SingleEndCI, nullptr);
3576+
EXPECT_EQ(SingleEndCI->getCalledFunction()->getName(), "__kmpc_end_single");
35513577
EXPECT_EQ(SingleEndCI->arg_size(), 2U);
35523578
EXPECT_TRUE(isa<GlobalVariable>(SingleEndCI->getArgOperand(0)));
35533579
EXPECT_EQ(SingleEndCI->getArgOperand(1), SingleEntryCI->getArgOperand(1));
3554-
3555-
CallInst *CopyPrivateCI = nullptr;
3556-
bool FoundBarrier = false;
3557-
for (auto &FI : *ExitBB) {
3558-
Instruction *Cur = &FI;
3559-
if (auto *CI = dyn_cast<CallInst>(Cur)) {
3560-
if (CI->getCalledFunction()->getName() == "__kmpc_barrier")
3561-
FoundBarrier = true;
3562-
else if (CI->getCalledFunction()->getName() == "__kmpc_copyprivate")
3563-
CopyPrivateCI = CI;
3564-
}
3565-
}
3566-
EXPECT_FALSE(FoundBarrier);
3580+
// br ExitBB
3581+
auto *ExitBBBI = ThenBBI.next<BranchInst>();
3582+
EXPECT_NE(ExitBBBI, nullptr);
3583+
EXPECT_TRUE(ExitBBBI->isUnconditional());
3584+
EXPECT_EQ(ExitBBBI->getOperand(0), ExitBB);
3585+
EXPECT_FALSE(ThenBBI.hasNext());
3586+
3587+
// check ExitBB
3588+
BBInstIter ExitBBI(ExitBB);
3589+
// call __kmpc_global_thread_num
3590+
auto *ThreadNumCI = ExitBBI.next<CallInst>();
3591+
EXPECT_NE(ThreadNumCI, nullptr);
3592+
EXPECT_EQ(ThreadNumCI->getCalledFunction()->getName(),
3593+
"__kmpc_global_thread_num");
3594+
// load DidIt
3595+
auto *DidItLI = ExitBBI.next<LoadInst>();
3596+
EXPECT_NE(DidItLI, nullptr);
3597+
EXPECT_EQ(DidItLI->getPointerOperand(), DidIt);
3598+
// call __kmpc_copyprivate
3599+
auto *CopyPrivateCI = ExitBBI.next<CallInst>();
35673600
EXPECT_NE(CopyPrivateCI, nullptr);
35683601
EXPECT_EQ(CopyPrivateCI->arg_size(), 6U);
35693602
EXPECT_TRUE(isa<AllocaInst>(CopyPrivateCI->getArgOperand(3)));
35703603
EXPECT_EQ(CopyPrivateCI->getArgOperand(3), CPVar);
35713604
EXPECT_TRUE(isa<Function>(CopyPrivateCI->getArgOperand(4)));
35723605
EXPECT_EQ(CopyPrivateCI->getArgOperand(4), CopyFunc);
35733606
EXPECT_TRUE(isa<LoadInst>(CopyPrivateCI->getArgOperand(5)));
3574-
LoadInst *DidItLI = cast<LoadInst>(CopyPrivateCI->getArgOperand(5));
3607+
DidItLI = cast<LoadInst>(CopyPrivateCI->getArgOperand(5));
35753608
EXPECT_EQ(DidItLI->getOperand(0), DidIt);
3609+
EXPECT_FALSE(ExitBBI.hasNext());
35763610
}
35773611

35783612
TEST_F(OpenMPIRBuilderTest, OMPAtomicReadFlt) {

‎mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -669,13 +669,9 @@ convertOmpSingle(omp::SingleOp &singleOp, llvm::IRBuilderBase &builder,
669669
llvmCPFuncs.push_back(
670670
moduleTranslation.lookupFunction(llvmFuncOp.getName()));
671671
}
672-
llvm::Value *didIt = nullptr;
673-
if (!llvmCPVars.empty())
674-
didIt = builder.CreateAlloca(llvm::Type::getInt32Ty(builder.getContext()));
675672

676673
builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createSingle(
677-
ompLoc, bodyCB, finiCB, singleOp.getNowait(), didIt, llvmCPVars,
678-
llvmCPFuncs));
674+
ompLoc, bodyCB, finiCB, singleOp.getNowait(), llvmCPVars, llvmCPFuncs));
679675
return bodyGenStatus;
680676
}
681677

0 commit comments

Comments
 (0)