[NFC][RemoveDIs] Switch constant-hoisting to insert with iterators #84738
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Seeing how constant-hoisting also happens to store an insertion-point in a DenseMap, this means we have to install DenseMapInfo for hashing BasicBlock::iterators and comparing them.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-ir Author: Jeremy Morse (jmorse) ChangesSeeing how constant-hoisting also happens to store an insertion-point in a DenseMap, this means we have to install DenseMapInfo for hashing BasicBlock::iterators and comparing them. I'm not really sure where to put the DenseMapInfo declarations as BasicBlock.h seems most logical, but that then means including DenseMap.h into pretty much all of LLVM. I've sent this up to the compile time tracker to see whether there's a major cost from this. Full diff: https://github.com/llvm/llvm-project/pull/84738.diff 3 Files Affected:
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index 179305e9260f91..1add4c2da9d438 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -14,6 +14,7 @@
#define LLVM_IR_BASICBLOCK_H
#include "llvm-c/Types.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/Twine.h"
#include "llvm/ADT/ilist.h"
#include "llvm/ADT/ilist_node.h"
@@ -774,6 +775,30 @@ BasicBlock::iterator skipDebugIntrinsics(BasicBlock::iterator It);
inline void BasicBlock::validateInstrOrdering() const {}
#endif
+// Specialize DenseMapInfo for iterators, so that ththey can be installed into
+// maps and sets. The iterator is made up of its node pointer, and the
+// debug-info "head" bit.
+template<>
+struct DenseMapInfo<BasicBlock::iterator> {
+ static inline BasicBlock::iterator getEmptyKey() {
+ return BasicBlock::iterator(nullptr);
+ }
+
+ static inline BasicBlock::iterator getTombstoneKey() {
+ BasicBlock::iterator It(nullptr);
+ It.setHeadBit(true);
+ return It;
+ }
+
+ static unsigned getHashValue(const BasicBlock::iterator &It) {
+ return DenseMapInfo<void *>::getHashValue(reinterpret_cast<void *>(It.getNodePtr())) ^ It.getHeadBit();
+ }
+
+ static bool isEqual(const BasicBlock::iterator &LHS, const BasicBlock::iterator &RHS) {
+ return LHS == RHS && LHS.getHeadBit() == RHS.getHeadBit();
+ }
+};
+
} // end namespace llvm
#endif // LLVM_IR_BASICBLOCK_H
diff --git a/llvm/include/llvm/Transforms/Scalar/ConstantHoisting.h b/llvm/include/llvm/Transforms/Scalar/ConstantHoisting.h
index a4d1653fdf4bc6..e2c165365bedc0 100644
--- a/llvm/include/llvm/Transforms/Scalar/ConstantHoisting.h
+++ b/llvm/include/llvm/Transforms/Scalar/ConstantHoisting.h
@@ -172,11 +172,11 @@ class ConstantHoistingPass : public PassInfoMixin<ConstantHoistingPass> {
void collectMatInsertPts(
const consthoist::RebasedConstantListType &RebasedConstants,
- SmallVectorImpl<Instruction *> &MatInsertPts) const;
- Instruction *findMatInsertPt(Instruction *Inst, unsigned Idx = ~0U) const;
- SetVector<Instruction *>
+ SmallVectorImpl<BasicBlock::iterator> &MatInsertPts) const;
+ BasicBlock::iterator findMatInsertPt(Instruction *Inst, unsigned Idx = ~0U) const;
+ SetVector<BasicBlock::iterator>
findConstantInsertionPoint(const consthoist::ConstantInfo &ConstInfo,
- const ArrayRef<Instruction *> MatInsertPts) const;
+ const ArrayRef<BasicBlock::iterator> MatInsertPts) const;
void collectConstantCandidates(ConstCandMapType &ConstCandMap,
Instruction *Inst, unsigned Idx,
ConstantInt *ConstInt);
@@ -203,9 +203,9 @@ class ConstantHoistingPass : public PassInfoMixin<ConstantHoistingPass> {
struct UserAdjustment {
Constant *Offset;
Type *Ty;
- Instruction *MatInsertPt;
+ BasicBlock::iterator MatInsertPt;
const consthoist::ConstantUser User;
- UserAdjustment(Constant *O, Type *T, Instruction *I,
+ UserAdjustment(Constant *O, Type *T, BasicBlock::iterator I,
consthoist::ConstantUser U)
: Offset(O), Ty(T), MatInsertPt(I), User(U) {}
};
diff --git a/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp b/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
index 49f8761a139232..6d1db436ecd9be 100644
--- a/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
@@ -162,14 +162,14 @@ bool ConstantHoistingLegacyPass::runOnFunction(Function &Fn) {
void ConstantHoistingPass::collectMatInsertPts(
const RebasedConstantListType &RebasedConstants,
- SmallVectorImpl<Instruction *> &MatInsertPts) const {
+ SmallVectorImpl<BasicBlock::iterator> &MatInsertPts) const {
for (const RebasedConstantInfo &RCI : RebasedConstants)
for (const ConstantUser &U : RCI.Uses)
MatInsertPts.emplace_back(findMatInsertPt(U.Inst, U.OpndIdx));
}
/// Find the constant materialization insertion point.
-Instruction *ConstantHoistingPass::findMatInsertPt(Instruction *Inst,
+BasicBlock::iterator ConstantHoistingPass::findMatInsertPt(Instruction *Inst,
unsigned Idx) const {
// If the operand is a cast instruction, then we have to materialize the
// constant before the cast instruction.
@@ -177,12 +177,12 @@ Instruction *ConstantHoistingPass::findMatInsertPt(Instruction *Inst,
Value *Opnd = Inst->getOperand(Idx);
if (auto CastInst = dyn_cast<Instruction>(Opnd))
if (CastInst->isCast())
- return CastInst;
+ return CastInst->getIterator();
}
// The simple and common case. This also includes constant expressions.
if (!isa<PHINode>(Inst) && !Inst->isEHPad())
- return Inst;
+ return Inst->getIterator();
// We can't insert directly before a phi node or an eh pad. Insert before
// the terminator of the incoming or dominating block.
@@ -191,7 +191,7 @@ Instruction *ConstantHoistingPass::findMatInsertPt(Instruction *Inst,
if (Idx != ~0U && isa<PHINode>(Inst)) {
InsertionBlock = cast<PHINode>(Inst)->getIncomingBlock(Idx);
if (!InsertionBlock->isEHPad()) {
- return InsertionBlock->getTerminator();
+ return InsertionBlock->getTerminator()->getIterator();
}
} else {
InsertionBlock = Inst->getParent();
@@ -206,7 +206,7 @@ Instruction *ConstantHoistingPass::findMatInsertPt(Instruction *Inst,
IDom = IDom->getIDom();
}
- return IDom->getBlock()->getTerminator();
+ return IDom->getBlock()->getTerminator()->getIterator();
}
/// Given \p BBs as input, find another set of BBs which collectively
@@ -314,26 +314,26 @@ static void findBestInsertionSet(DominatorTree &DT, BlockFrequencyInfo &BFI,
}
/// Find an insertion point that dominates all uses.
-SetVector<Instruction *> ConstantHoistingPass::findConstantInsertionPoint(
+SetVector<BasicBlock::iterator> ConstantHoistingPass::findConstantInsertionPoint(
const ConstantInfo &ConstInfo,
- const ArrayRef<Instruction *> MatInsertPts) const {
+ const ArrayRef<BasicBlock::iterator> MatInsertPts) const {
assert(!ConstInfo.RebasedConstants.empty() && "Invalid constant info entry.");
// Collect all basic blocks.
SetVector<BasicBlock *> BBs;
- SetVector<Instruction *> InsertPts;
+ SetVector<BasicBlock::iterator> InsertPts;
- for (Instruction *MatInsertPt : MatInsertPts)
+ for (BasicBlock::iterator MatInsertPt : MatInsertPts)
BBs.insert(MatInsertPt->getParent());
if (BBs.count(Entry)) {
- InsertPts.insert(&Entry->front());
+ InsertPts.insert(Entry->begin());
return InsertPts;
}
if (BFI) {
findBestInsertionSet(*DT, *BFI, Entry, BBs);
for (BasicBlock *BB : BBs)
- InsertPts.insert(&*BB->getFirstInsertionPt());
+ InsertPts.insert(BB->getFirstInsertionPt());
return InsertPts;
}
@@ -343,7 +343,7 @@ SetVector<Instruction *> ConstantHoistingPass::findConstantInsertionPoint(
BB2 = BBs.pop_back_val();
BB = DT->findNearestCommonDominator(BB1, BB2);
if (BB == Entry) {
- InsertPts.insert(&Entry->front());
+ InsertPts.insert(Entry->begin());
return InsertPts;
}
BBs.insert(BB);
@@ -761,11 +761,11 @@ void ConstantHoistingPass::emitBaseConstants(Instruction *Base,
Mat = GetElementPtrInst::Create(Type::getInt8Ty(*Ctx), Base, Adj->Offset,
"mat_gep", Adj->MatInsertPt);
// Hide it behind a bitcast.
- Mat = new BitCastInst(Mat, Adj->Ty, "mat_bitcast", Adj->MatInsertPt);
+ Mat = new BitCastInst(Mat, Adj->Ty, "mat_bitcast", Adj->MatInsertPt->getIterator());
} else
// Constant being rebased is a ConstantInt.
Mat = BinaryOperator::Create(Instruction::Add, Base, Adj->Offset,
- "const_mat", Adj->MatInsertPt);
+ "const_mat", Adj->MatInsertPt->getIterator());
LLVM_DEBUG(dbgs() << "Materialize constant (" << *Base->getOperand(0)
<< " + " << *Adj->Offset << ") in BB "
@@ -816,7 +816,8 @@ void ConstantHoistingPass::emitBaseConstants(Instruction *Base,
// Aside from constant GEPs, only constant cast expressions are collected.
assert(ConstExpr->isCast() && "ConstExpr should be a cast");
- Instruction *ConstExprInst = ConstExpr->getAsInstruction(Adj->MatInsertPt);
+ Instruction *ConstExprInst = ConstExpr->getAsInstruction();
+ ConstExprInst->insertBefore(Adj->MatInsertPt);
ConstExprInst->setOperand(0, Mat);
// Use the same debug location as the instruction we are about to update.
@@ -842,9 +843,9 @@ bool ConstantHoistingPass::emitBaseConstants(GlobalVariable *BaseGV) {
SmallVectorImpl<consthoist::ConstantInfo> &ConstInfoVec =
BaseGV ? ConstGEPInfoMap[BaseGV] : ConstIntInfoVec;
for (const consthoist::ConstantInfo &ConstInfo : ConstInfoVec) {
- SmallVector<Instruction *, 4> MatInsertPts;
+ SmallVector<BasicBlock::iterator, 4> MatInsertPts;
collectMatInsertPts(ConstInfo.RebasedConstants, MatInsertPts);
- SetVector<Instruction *> IPSet =
+ SetVector<BasicBlock::iterator> IPSet =
findConstantInsertionPoint(ConstInfo, MatInsertPts);
// We can have an empty set if the function contains unreachable blocks.
if (IPSet.empty())
@@ -853,7 +854,7 @@ bool ConstantHoistingPass::emitBaseConstants(GlobalVariable *BaseGV) {
unsigned UsesNum = 0;
unsigned ReBasesNum = 0;
unsigned NotRebasedNum = 0;
- for (Instruction *IP : IPSet) {
+ for (const BasicBlock::iterator &IP : IPSet) {
// First, collect constants depending on this IP of the base.
UsesNum = 0;
SmallVector<UserAdjustment, 4> ToBeRebased;
@@ -861,7 +862,7 @@ bool ConstantHoistingPass::emitBaseConstants(GlobalVariable *BaseGV) {
for (auto const &RCI : ConstInfo.RebasedConstants) {
UsesNum += RCI.Uses.size();
for (auto const &U : RCI.Uses) {
- Instruction *MatInsertPt = MatInsertPts[MatCtr++];
+ const BasicBlock::iterator &MatInsertPt = MatInsertPts[MatCtr++];
BasicBlock *OrigMatInsertBB = MatInsertPt->getParent();
// If Base constant is to be inserted in multiple places,
// generate rebase for U using the Base dominating U.
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
OCHyams
approved these changes
Mar 11, 2024
tl;dr no noticeable impact! |
chencha3
pushed a commit
to chencha3/llvm-project
that referenced
this pull request
Mar 23, 2024
…lvm#84738) Seeing how constant-hoisting also happens to store an insertion-point in a DenseMap, this means we have to install DenseMapInfo for hashing BasicBlock::iterators and comparing them. I'm not really sure where to put the DenseMapInfo declarations as BasicBlock.h seems most logical, but that then means including DenseMap.h into pretty much all of LLVM. I've sent this up to the compile time tracker to see whether there's a major cost from this. --------- Merged by: Stephen Tozer <stephen.tozer@sony.com>
SevenIsSeven
pushed a commit
to SevenIsSeven/llvm-project
that referenced
this pull request
Mar 26, 2024
This commit(llvm#84738) introduced following compile warning then treated-as-error in MSVC build. >>>"'^': unsafe mix of type 'unsigned int' and type 'bool' in operation"
This was referenced Mar 26, 2024
ScottTodd
reviewed
Mar 26, 2024
Comment on lines
+792
to
+794
| return DenseMapInfo<void *>::getHashValue( | ||
| reinterpret_cast<void *>(It.getNodePtr())) ^ | ||
| It.getHeadBit(); |
There was a problem hiding this comment.
This triggers warnings on MSVC: llvm-project\llvm\include\llvm/IR/BasicBlock.h(794): warning C4805: '^': unsafe mix of type 'unsigned int' and type 'bool' in operation
There was a problem hiding this comment.
Ah, there are PRs for that already.
ScottTodd
pushed a commit
that referenced
this pull request
Mar 27, 2024
This commit(#84738) introduced following compile warning then treated-as-error in MSVC build. >>>"'^': unsafe mix of type 'unsigned int' and type 'bool' in operation" --------- Co-authored-by: Seven <Seven.Li@amd.com>
ScottTodd
pushed a commit
to iree-org/llvm-project
that referenced
this pull request
Mar 27, 2024
This commit(llvm#84738) introduced following compile warning then treated-as-error in MSVC build. >>>"'^': unsafe mix of type 'unsigned int' and type 'bool' in operation" --------- Co-authored-by: Seven <Seven.Li@amd.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Seeing how constant-hoisting also happens to store an insertion-point in a DenseMap, this means we have to install DenseMapInfo for hashing BasicBlock::iterators and comparing them. I'm not really sure where to put the DenseMapInfo declarations as BasicBlock.h seems most logical, but that then means including DenseMap.h into pretty much all of LLVM. I've sent this up to the compile time tracker to see whether there's a major cost from this.