From 35dfa78ff8d44733b1c805309f0bbd4a8c960897 Mon Sep 17 00:00:00 2001 From: Shraiysh Vaishay Date: Thu, 10 Feb 2022 10:38:06 +0530 Subject: [PATCH] [OpenMP][IRBuilder] Handle floats for atomic update and fix AllocaIP for update/capture This patch fixes `createAtomicUpdate` for lowering with float types. Test added for the same. This patch also changes the alloca argument for createAtomicUpdate and createAtomicCapture from `Instruction*` to `InsertPointTy`. This is in line with the other functions of the OpenMPIRBuilder class which take AllocaIP as an `InsertPointTy`. Reviewed By: Meinersbur Differential Revision: https://reviews.llvm.org/D118227 --- llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | 13 ++-- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 29 +++++---- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp | 82 ++++++++++++++++++++++-- 3 files changed, 100 insertions(+), 24 deletions(-) diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h index 18a88ec..7643da2 100644 --- a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h +++ b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h @@ -1214,7 +1214,8 @@ private: /// For complex Operations: X = UpdateOp(X) => CmpExch X, old_X, UpdateOp(X) /// Only Scalar data types. /// - /// \param AllocIP Instruction to create AllocaInst before. + /// \param AllocaIP The insertion point to be used for alloca + /// instructions. /// \param X The target atomic pointer to be updated /// \param XElemTy The element type of the atomic pointer. /// \param Expr The value to update X with. @@ -1234,7 +1235,7 @@ private: /// \returns A pair of the old value of X before the update, and the value /// used for the update. std::pair - emitAtomicUpdate(Instruction *AllocIP, Value *X, Type *XElemTy, Value *Expr, + emitAtomicUpdate(InsertPointTy AllocaIP, Value *X, Type *XElemTy, Value *Expr, AtomicOrdering AO, AtomicRMWInst::BinOp RMWOp, AtomicUpdateCallbackTy &UpdateOp, bool VolatileX, bool IsXBinopExpr); @@ -1286,7 +1287,7 @@ public: /// Only Scalar data types. /// /// \param Loc The insert and source location description. - /// \param AllocIP Instruction to create AllocaInst before. + /// \param AllocaIP The insertion point to be used for alloca instructions. /// \param X The target atomic pointer to be updated /// \param Expr The value to update X with. /// \param AO Atomic ordering of the generated atomic instructions. @@ -1302,7 +1303,7 @@ public: /// /// \return Insertion point after generated atomic update IR. InsertPointTy createAtomicUpdate(const LocationDescription &Loc, - Instruction *AllocIP, AtomicOpValue &X, + InsertPointTy AllocaIP, AtomicOpValue &X, Value *Expr, AtomicOrdering AO, AtomicRMWInst::BinOp RMWOp, AtomicUpdateCallbackTy &UpdateOp, @@ -1317,7 +1318,7 @@ public: /// X = UpdateOp(X); V = X, /// /// \param Loc The insert and source location description. - /// \param AllocIP Instruction to create AllocaInst before. + /// \param AllocaIP The insertion point to be used for alloca instructions. /// \param X The target atomic pointer to be updated /// \param V Memory address where to store captured value /// \param Expr The value to update X with. @@ -1338,7 +1339,7 @@ public: /// /// \return Insertion point after generated atomic capture IR. InsertPointTy - createAtomicCapture(const LocationDescription &Loc, Instruction *AllocIP, + createAtomicCapture(const LocationDescription &Loc, InsertPointTy AllocaIP, AtomicOpValue &X, AtomicOpValue &V, Value *Expr, AtomicOrdering AO, AtomicRMWInst::BinOp RMWOp, AtomicUpdateCallbackTy &UpdateOp, bool UpdateExpr, diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index 476686d..293f956 100644 --- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -3291,9 +3291,10 @@ OpenMPIRBuilder::createAtomicWrite(const LocationDescription &Loc, } OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createAtomicUpdate( - const LocationDescription &Loc, Instruction *AllocIP, AtomicOpValue &X, + const LocationDescription &Loc, InsertPointTy AllocaIP, AtomicOpValue &X, Value *Expr, AtomicOrdering AO, AtomicRMWInst::BinOp RMWOp, AtomicUpdateCallbackTy &UpdateOp, bool IsXBinopExpr) { + assert(!isConflictIP(Loc.IP, AllocaIP) && "IPs must not be ambiguous"); if (!updateToLocation(Loc)) return Loc.IP; @@ -3310,7 +3311,7 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createAtomicUpdate( "OpenMP atomic does not support LT or GT operations"); }); - emitAtomicUpdate(AllocIP, X.Var, X.ElemTy, Expr, AO, RMWOp, UpdateOp, + emitAtomicUpdate(AllocaIP, X.Var, X.ElemTy, Expr, AO, RMWOp, UpdateOp, X.IsVolatile, IsXBinopExpr); checkAndEmitFlushAfterAtomic(Loc, AO, AtomicKind::Update); return Builder.saveIP(); @@ -3345,13 +3346,13 @@ Value *OpenMPIRBuilder::emitRMWOpAsInstruction(Value *Src1, Value *Src2, } std::pair OpenMPIRBuilder::emitAtomicUpdate( - Instruction *AllocIP, Value *X, Type *XElemTy, Value *Expr, + InsertPointTy AllocaIP, Value *X, Type *XElemTy, Value *Expr, AtomicOrdering AO, AtomicRMWInst::BinOp RMWOp, AtomicUpdateCallbackTy &UpdateOp, bool VolatileX, bool IsXBinopExpr) { - bool DoCmpExch = - ((RMWOp == AtomicRMWInst::BAD_BINOP) || (RMWOp == AtomicRMWInst::FAdd)) || - (RMWOp == AtomicRMWInst::FSub) || - (RMWOp == AtomicRMWInst::Sub && !IsXBinopExpr); + bool DoCmpExch = (RMWOp == AtomicRMWInst::BAD_BINOP) || + (RMWOp == AtomicRMWInst::FAdd) || + (RMWOp == AtomicRMWInst::FSub) || + (RMWOp == AtomicRMWInst::Sub && !IsXBinopExpr) || !XElemTy; std::pair Res; if (XElemTy->isIntegerTy() && !DoCmpExch) { @@ -3381,12 +3382,12 @@ std::pair OpenMPIRBuilder::emitAtomicUpdate( BasicBlock *ContBB = CurBB->splitBasicBlock(CurBB->getTerminator(), X->getName() + ".atomic.cont"); ContBB->getTerminator()->eraseFromParent(); + Builder.restoreIP(AllocaIP); + AllocaInst *NewAtomicAddr = Builder.CreateAlloca(XElemTy); + NewAtomicAddr->setName(X->getName() + "x.new.val"); Builder.SetInsertPoint(ContBB); llvm::PHINode *PHI = Builder.CreatePHI(OldVal->getType(), 2); PHI->addIncoming(OldVal, CurBB); - AllocaInst *NewAtomicAddr = Builder.CreateAlloca(XElemTy); - NewAtomicAddr->setName(X->getName() + "x.new.val"); - NewAtomicAddr->moveBefore(AllocIP); IntegerType *NewAtomicCastTy = IntegerType::get(M.getContext(), XElemTy->getScalarSizeInBits()); bool IsIntTy = XElemTy->isIntegerTy(); @@ -3408,7 +3409,7 @@ std::pair OpenMPIRBuilder::emitAtomicUpdate( Value *Upd = UpdateOp(OldExprVal, Builder); Builder.CreateStore(Upd, NewAtomicAddr); - LoadInst *DesiredVal = Builder.CreateLoad(XElemTy, NewAtomicIntAddr); + LoadInst *DesiredVal = Builder.CreateLoad(IntCastTy, NewAtomicIntAddr); Value *XAddr = (IsIntTy) ? X @@ -3416,7 +3417,7 @@ std::pair OpenMPIRBuilder::emitAtomicUpdate( AtomicOrdering Failure = llvm::AtomicCmpXchgInst::getStrongestFailureOrdering(AO); AtomicCmpXchgInst *Result = Builder.CreateAtomicCmpXchg( - XAddr, OldExprVal, DesiredVal, llvm::MaybeAlign(), AO, Failure); + XAddr, PHI, DesiredVal, llvm::MaybeAlign(), AO, Failure); Result->setVolatile(VolatileX); Value *PreviousVal = Builder.CreateExtractValue(Result, /*Idxs=*/0); Value *SuccessFailureVal = Builder.CreateExtractValue(Result, /*Idxs=*/1); @@ -3440,7 +3441,7 @@ std::pair OpenMPIRBuilder::emitAtomicUpdate( } OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createAtomicCapture( - const LocationDescription &Loc, Instruction *AllocIP, AtomicOpValue &X, + const LocationDescription &Loc, InsertPointTy AllocaIP, AtomicOpValue &X, AtomicOpValue &V, Value *Expr, AtomicOrdering AO, AtomicRMWInst::BinOp RMWOp, AtomicUpdateCallbackTy &UpdateOp, bool UpdateExpr, bool IsPostfixUpdate, bool IsXBinopExpr) { @@ -3463,7 +3464,7 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createAtomicCapture( // 'x' is simply atomically rewritten with 'expr'. AtomicRMWInst::BinOp AtomicOp = (UpdateExpr ? RMWOp : AtomicRMWInst::Xchg); std::pair Result = - emitAtomicUpdate(AllocIP, X.Var, X.ElemTy, Expr, AO, AtomicOp, UpdateOp, + emitAtomicUpdate(AllocaIP, X.Var, X.ElemTy, Expr, AO, AtomicOp, UpdateOp, X.IsVolatile, IsXBinopExpr); Value *CapturedVal = (IsPostfixUpdate ? Result.first : Result.second); diff --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp index f5fd7de..48f720b 100644 --- a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp +++ b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp @@ -2936,7 +2936,8 @@ TEST_F(OpenMPIRBuilderTest, OMPAtomicUpdate) { bool IsXLHSInRHSPart = false; BasicBlock *EntryBB = BB; - Instruction *AllocIP = EntryBB->getFirstNonPHI(); + OpenMPIRBuilder::InsertPointTy AllocaIP(EntryBB, + EntryBB->getFirstInsertionPt()); Value *Sub = nullptr; auto UpdateOp = [&](Value *Atomic, IRBuilder<> &IRB) { @@ -2944,7 +2945,7 @@ TEST_F(OpenMPIRBuilderTest, OMPAtomicUpdate) { return Sub; }; Builder.restoreIP(OMPBuilder.createAtomicUpdate( - Builder, AllocIP, X, Expr, AO, RMWOp, UpdateOp, IsXLHSInRHSPart)); + Builder, AllocaIP, X, Expr, AO, RMWOp, UpdateOp, IsXLHSInRHSPart)); BasicBlock *ContBB = EntryBB->getSingleSuccessor(); BranchInst *ContTI = dyn_cast(ContBB->getTerminator()); EXPECT_NE(ContTI, nullptr); @@ -2982,6 +2983,78 @@ TEST_F(OpenMPIRBuilderTest, OMPAtomicUpdate) { EXPECT_FALSE(verifyModule(*M, &errs())); } +TEST_F(OpenMPIRBuilderTest, OMPAtomicUpdateFloat) { + OpenMPIRBuilder OMPBuilder(*M); + OMPBuilder.initialize(); + F->setName("func"); + IRBuilder<> Builder(BB); + + OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL}); + + Type *FloatTy = Type::getFloatTy(M->getContext()); + AllocaInst *XVal = Builder.CreateAlloca(FloatTy); + XVal->setName("AtomicVar"); + Builder.CreateStore(ConstantFP::get(Type::getFloatTy(Ctx), 0.0), XVal); + OpenMPIRBuilder::AtomicOpValue X = {XVal, FloatTy, false, false}; + AtomicOrdering AO = AtomicOrdering::Monotonic; + Constant *ConstVal = ConstantFP::get(Type::getFloatTy(Ctx), 1.0); + Value *Expr = nullptr; + AtomicRMWInst::BinOp RMWOp = AtomicRMWInst::FSub; + bool IsXLHSInRHSPart = false; + + BasicBlock *EntryBB = BB; + OpenMPIRBuilder::InsertPointTy AllocaIP(EntryBB, + EntryBB->getFirstInsertionPt()); + Value *Sub = nullptr; + + auto UpdateOp = [&](Value *Atomic, IRBuilder<> &IRB) { + Sub = IRB.CreateFSub(ConstVal, Atomic); + return Sub; + }; + Builder.restoreIP(OMPBuilder.createAtomicUpdate( + Builder, AllocaIP, X, Expr, AO, RMWOp, UpdateOp, IsXLHSInRHSPart)); + BasicBlock *ContBB = EntryBB->getSingleSuccessor(); + BranchInst *ContTI = dyn_cast(ContBB->getTerminator()); + EXPECT_NE(ContTI, nullptr); + BasicBlock *EndBB = ContTI->getSuccessor(0); + EXPECT_TRUE(ContTI->isConditional()); + EXPECT_EQ(ContTI->getSuccessor(1), ContBB); + EXPECT_NE(EndBB, nullptr); + + PHINode *Phi = dyn_cast(&ContBB->front()); + EXPECT_NE(Phi, nullptr); + EXPECT_EQ(Phi->getNumIncomingValues(), 2U); + EXPECT_EQ(Phi->getIncomingBlock(0), EntryBB); + EXPECT_EQ(Phi->getIncomingBlock(1), ContBB); + + EXPECT_EQ(Sub->getNumUses(), 1U); + StoreInst *St = dyn_cast(Sub->user_back()); + AllocaInst *UpdateTemp = dyn_cast(St->getPointerOperand()); + + ExtractValueInst *ExVI1 = + dyn_cast(Phi->getIncomingValueForBlock(ContBB)); + EXPECT_NE(ExVI1, nullptr); + AtomicCmpXchgInst *CmpExchg = + dyn_cast(ExVI1->getAggregateOperand()); + EXPECT_NE(CmpExchg, nullptr); + BitCastInst *BitCastNew = + dyn_cast(CmpExchg->getPointerOperand()); + EXPECT_NE(BitCastNew, nullptr); + EXPECT_EQ(BitCastNew->getOperand(0), XVal); + EXPECT_EQ(CmpExchg->getCompareOperand(), Phi); + EXPECT_EQ(CmpExchg->getSuccessOrdering(), AtomicOrdering::Monotonic); + + LoadInst *Ld = dyn_cast(CmpExchg->getNewValOperand()); + EXPECT_NE(Ld, nullptr); + BitCastInst *BitCastOld = dyn_cast(Ld->getPointerOperand()); + EXPECT_NE(BitCastOld, nullptr); + EXPECT_EQ(UpdateTemp, BitCastOld->getOperand(0)); + + Builder.CreateRetVoid(); + OMPBuilder.finalize(); + EXPECT_FALSE(verifyModule(*M, &errs())); +} + TEST_F(OpenMPIRBuilderTest, OMPAtomicCapture) { OpenMPIRBuilder OMPBuilder(*M); OMPBuilder.initialize(); @@ -3009,13 +3082,14 @@ TEST_F(OpenMPIRBuilderTest, OMPAtomicCapture) { bool UpdateExpr = true; BasicBlock *EntryBB = BB; - Instruction *AllocIP = EntryBB->getFirstNonPHI(); + OpenMPIRBuilder::InsertPointTy AllocaIP(EntryBB, + EntryBB->getFirstInsertionPt()); // integer update - not used auto UpdateOp = [&](Value *Atomic, IRBuilder<> &IRB) { return nullptr; }; Builder.restoreIP(OMPBuilder.createAtomicCapture( - Builder, AllocIP, X, V, Expr, AO, RMWOp, UpdateOp, UpdateExpr, + Builder, AllocaIP, X, V, Expr, AO, RMWOp, UpdateOp, UpdateExpr, IsPostfixUpdate, IsXLHSInRHSPart)); EXPECT_EQ(EntryBB->getParent()->size(), 1U); AtomicRMWInst *ARWM = dyn_cast(Init->getNextNode()); -- 2.7.4