[NFC][RemoveDIs] Switch ConstantExpr::getAsInstruction to not insert

[jmorse]

Because the RemoveDIs work is putting a debug-info bit into BasicBlock::iterator and iterators are needed for insertion, the getAsInstruction method declaration would need to use a fully defined instruction-iterator, which leads to a complicated header-inclusion-order problem. Much simpler to instead just not insert, and make it the callers problem to insert.

This is proportionate because there are only four call-sites to getAsInstruction -- it would suck if we did this everywhere.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-flang-openmp

Author: Jeremy Morse (jmorse)

Changes

Because the RemoveDIs work is putting a debug-info bit into BasicBlock::iterator and iterators are needed for insertion, the getAsInstruction method declaration would need to use a fully defined instruction-iterator, which leads to a complicated header-inclusion-order problem. Much simpler to instead just not insert, and make it the callers problem to insert.

This is proportionate because there are only four call-sites to getAsInstruction -- it would suck if we did this everywhere.

---

Full diff: https://github.com/llvm/llvm-project/pull/84737.diff

5 Files Affected:

• (modified) llvm/include/llvm/IR/Constants.h (+2-3)
• (modified) llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp (+9-4)
• (modified) llvm/lib/IR/Constants.cpp (+9-10)
• (modified) llvm/lib/IR/ReplaceConstant.cpp (+7-6)
• (modified) llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp (+5-3)

diff --git a/llvm/include/llvm/IR/Constants.h b/llvm/include/llvm/IR/Constants.h index c0ac9a4aa6750c..c5e1e19d649824 100644 --- a/llvm/include/llvm/IR/Constants.h +++ b/llvm/include/llvm/IR/Constants.h @@ -1289,14 +1289,13 @@ class ConstantExpr : public Constant { Type *SrcTy = nullptr) const; /// Returns an Instruction which implements the same operation as this - /// ConstantExpr. If \p InsertBefore is not null, the new instruction is - /// inserted before it, otherwise it is not inserted into any basic block. + /// ConstantExpr. It is not inserted into any basic block. /// /// A better approach to this could be to have a constructor for Instruction /// which would take a ConstantExpr parameter, but that would have spread /// implementation details of ConstantExpr outside of Constants.cpp, which /// would make it harder to remove ConstantExprs altogether. - Instruction *getAsInstruction(Instruction *InsertBefore = nullptr) const; + Instruction *getAsInstruction() const; /// Whether creating a constant expression for this binary operator is /// desirable. diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp index d65ed8c11d86cc..585e08f9c25615 100644 --- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp @@ -5056,10 +5056,15 @@ FunctionCallee OpenMPIRBuilder::createDispatchFiniFunction(unsigned IVSize, static void replaceConstatExprUsesInFuncWithInstr(ConstantExpr *ConstExpr, Function *Func) { - for (User *User : make_early_inc_range(ConstExpr->users())) - if (auto *Instr = dyn_cast<Instruction>(User)) - if (Instr->getFunction() == Func) - Instr->replaceUsesOfWith(ConstExpr, ConstExpr->getAsInstruction(Instr)); + for (User *User : make_early_inc_range(ConstExpr->users())) { + if (auto *Instr = dyn_cast<Instruction>(User)) { + if (Instr->getFunction() == Func) { + Instruction *ConstInst = ConstExpr->getAsInstruction(); + ConstInst->insertBefore(*Instr->getParent(), Instr->getIterator()); + Instr->replaceUsesOfWith(ConstExpr, ConstInst); + } + } + } } static void replaceConstantValueUsesInFuncWithInstr(llvm::Value *Input, diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp index e6b92aad392f66..111f6bc54b6808 100644 --- a/llvm/lib/IR/Constants.cpp +++ b/llvm/lib/IR/Constants.cpp @@ -3303,7 +3303,7 @@ Value *ConstantExpr::handleOperandChangeImpl(Value *From, Value *ToV) { NewOps, this, From, To, NumUpdated, OperandNo); } -Instruction *ConstantExpr::getAsInstruction(Instruction *InsertBefore) const { +Instruction *ConstantExpr::getAsInstruction() const { SmallVector<Value *, 4> ValueOperands(operands()); ArrayRef<Value*> Ops(ValueOperands); @@ -3322,32 +3322,31 @@ Instruction *ConstantExpr::getAsInstruction(Instruction *InsertBefore) const { case Instruction::BitCast: case Instruction::AddrSpaceCast: return CastInst::Create((Instruction::CastOps)getOpcode(), Ops[0], - getType(), "", InsertBefore); + getType(), ""); case Instruction::InsertElement: - return InsertElementInst::Create(Ops[0], Ops[1], Ops[2], "", InsertBefore); + return InsertElementInst::Create(Ops[0], Ops[1], Ops[2], ""); case Instruction::ExtractElement: - return ExtractElementInst::Create(Ops[0], Ops[1], "", InsertBefore); + return ExtractElementInst::Create(Ops[0], Ops[1], ""); case Instruction::ShuffleVector: - return new ShuffleVectorInst(Ops[0], Ops[1], getShuffleMask(), "", - InsertBefore); + return new ShuffleVectorInst(Ops[0], Ops[1], getShuffleMask(), ""); case Instruction::GetElementPtr: { const auto *GO = cast<GEPOperator>(this); if (GO->isInBounds()) return GetElementPtrInst::CreateInBounds( - GO->getSourceElementType(), Ops[0], Ops.slice(1), "", InsertBefore); + GO->getSourceElementType(), Ops[0], Ops.slice(1), ""); return GetElementPtrInst::Create(GO->getSourceElementType(), Ops[0], - Ops.slice(1), "", InsertBefore); + Ops.slice(1), ""); } case Instruction::ICmp: case Instruction::FCmp: return CmpInst::Create((Instruction::OtherOps)getOpcode(), (CmpInst::Predicate)getPredicate(), Ops[0], Ops[1], - "", InsertBefore); + ""); default: assert(getNumOperands() == 2 && "Must be binary operator?"); BinaryOperator *BO = BinaryOperator::Create( - (Instruction::BinaryOps)getOpcode(), Ops[0], Ops[1], "", InsertBefore); + (Instruction::BinaryOps)getOpcode(), Ops[0], Ops[1], ""); if (isa<OverflowingBinaryOperator>(BO)) { BO->setHasNoUnsignedWrap(SubclassOptionalData & OverflowingBinaryOperator::NoUnsignedWrap); diff --git a/llvm/lib/IR/ReplaceConstant.cpp b/llvm/lib/IR/ReplaceConstant.cpp index 42dec7c72328ea..9b07bd8040492a 100644 --- a/llvm/lib/IR/ReplaceConstant.cpp +++ b/llvm/lib/IR/ReplaceConstant.cpp @@ -22,11 +22,13 @@ static bool isExpandableUser(User *U) { return isa<ConstantExpr>(U) || isa<ConstantAggregate>(U); } -static SmallVector<Instruction *, 4> expandUser(Instruction *InsertPt, +static SmallVector<Instruction *, 4> expandUser(BasicBlock::iterator InsertPt, Constant *C) { SmallVector<Instruction *, 4> NewInsts; if (auto *CE = dyn_cast<ConstantExpr>(C)) { - NewInsts.push_back(CE->getAsInstruction(InsertPt)); + Instruction *ConstInst = CE->getAsInstruction(); + ConstInst->insertBefore(*InsertPt->getParent(), InsertPt); + NewInsts.push_back(ConstInst); } else if (isa<ConstantStruct>(C) || isa<ConstantArray>(C)) { Value *V = PoisonValue::get(C->getType()); for (auto [Idx, Op] : enumerate(C->operands())) { @@ -80,12 +82,11 @@ bool convertUsersOfConstantsToInstructions(ArrayRef<Constant *> Consts) { Instruction *I = InstructionWorklist.pop_back_val(); DebugLoc Loc = I->getDebugLoc(); for (Use &U : I->operands()) { - auto *BI = I; + BasicBlock::iterator BI = I->getIterator(); if (auto *Phi = dyn_cast<PHINode>(I)) { BasicBlock *BB = Phi->getIncomingBlock(U); - BasicBlock::iterator It = BB->getFirstInsertionPt(); - assert(It != BB->end() && "Unexpected empty basic block"); - BI = &*It; + BI = BB->getFirstInsertionPt(); + assert(BI != BB->end() && "Unexpected empty basic block"); } if (auto *C = dyn_cast<Constant>(U.get())) { diff --git a/llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp b/llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp index b5a683de33ab13..0386e3cb9c5c36 100644 --- a/llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp +++ b/llvm/lib/Target/XCore/XCoreLowerThreadLocal.cpp @@ -88,12 +88,14 @@ static bool replaceConstantExprOp(ConstantExpr *CE, Pass *P) { BasicBlock *PredBB = PN->getIncomingBlock(I); if (PredBB->getTerminator()->getNumSuccessors() > 1) PredBB = SplitEdge(PredBB, PN->getParent()); - Instruction *InsertPos = PredBB->getTerminator(); - Instruction *NewInst = CE->getAsInstruction(InsertPos); + BasicBlock::iterator InsertPos = PredBB->getTerminator()->getIterator(); + Instruction *NewInst = CE->getAsInstruction(); + NewInst->insertBefore(*PredBB, InsertPos); PN->setOperand(I, NewInst); } } else if (Instruction *Instr = dyn_cast<Instruction>(WU)) { - Instruction *NewInst = CE->getAsInstruction(Instr); + Instruction *NewInst = CE->getAsInstruction(); + NewInst->insertBefore(*Instr->getParent(), Instr->getIterator()); Instr->replaceUsesOfWith(CE, NewInst); } else { ConstantExpr *CExpr = dyn_cast<ConstantExpr>(WU); 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[OCHyams]

Makes sense, LGTM given the low call site count