[llvm] [InstrRef][nfc] Remove usage of unique_ptrs of arrays (PR #74203)
Felipe de Azevedo Piovezan via llvm-commits
llvm-commits at lists.llvm.org
Sat Dec 2 10:35:06 PST 2023
https://github.com/felipepiovezan created https://github.com/llvm/llvm-project/pull/74203
These are usually difficult to reason about, and they were being used to pass raw pointers around with array semantic (i.e., we were using operator [] on raw pointers). To put it in InstrRef terminology: we were passing a pointer to a ValueTable but using it as if it were a FuncValueTable.
These could have easily been SmallVectors, which now allow us to have reference semantics in some places, as well as simpler initialization.
In the future, we can use even more pass-by-reference with some extra changes in the code.
>From 0e36c605e08612acd14878185a1dc5be9d2dc49b Mon Sep 17 00:00:00 2001
From: Felipe de Azevedo Piovezan <fpiovezan at apple.com>
Date: Sat, 2 Dec 2023 10:28:56 -0800
Subject: [PATCH] [InstrRef][nfc] Remove usage of unique_ptrs of arrays
These are usually difficult to reason about, and they were being used to pass
raw pointers around with array semantic (i.e., we were using operator [] on raw
pointers). To put it in InstrRef terminology: we were passing a pointer to a
ValueTable but using it as if it were a FuncValueTable.
These could have easily been SmallVectors, which now allow us to have
reference semantics in some places, as well as simpler initialization.
In the future, we can use even more pass-by-reference with some extra changes in
the code.
---
.../LiveDebugValues/InstrRefBasedImpl.cpp | 52 ++++++++-----------
.../LiveDebugValues/InstrRefBasedImpl.h | 24 ++++-----
llvm/unittests/CodeGen/InstrRefLDVTest.cpp | 11 +---
3 files changed, 37 insertions(+), 50 deletions(-)
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index b2c2b40139eda..cb0954dc544eb 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -1429,7 +1429,7 @@ bool InstrRefBasedLDV::transferDebugValue(const MachineInstr &MI) {
std::optional<ValueIDNum> InstrRefBasedLDV::getValueForInstrRef(
unsigned InstNo, unsigned OpNo, MachineInstr &MI,
- const ValueTable *MLiveOuts, const ValueTable *MLiveIns) {
+ const FuncValueTable *MLiveOuts, const FuncValueTable *MLiveIns) {
// Various optimizations may have happened to the value during codegen,
// recorded in the value substitution table. Apply any substitutions to
// the instruction / operand number in this DBG_INSTR_REF, and collect
@@ -1495,7 +1495,8 @@ std::optional<ValueIDNum> InstrRefBasedLDV::getValueForInstrRef(
} else if (PHIIt != DebugPHINumToValue.end() && PHIIt->InstrNum == InstNo) {
// It's actually a PHI value. Which value it is might not be obvious, use
// the resolver helper to find out.
- NewID = resolveDbgPHIs(*MI.getParent()->getParent(), MLiveOuts, MLiveIns,
+ assert(MLiveOuts && MLiveIns);
+ NewID = resolveDbgPHIs(*MI.getParent()->getParent(), *MLiveOuts, *MLiveIns,
MI, InstNo);
}
@@ -1574,8 +1575,8 @@ std::optional<ValueIDNum> InstrRefBasedLDV::getValueForInstrRef(
}
bool InstrRefBasedLDV::transferDebugInstrRef(MachineInstr &MI,
- const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns) {
+ const FuncValueTable *MLiveOuts,
+ const FuncValueTable *MLiveIns) {
if (!MI.isDebugRef())
return false;
@@ -2245,8 +2246,8 @@ void InstrRefBasedLDV::accumulateFragmentMap(MachineInstr &MI) {
AllSeenFragments.insert(ThisFragment);
}
-void InstrRefBasedLDV::process(MachineInstr &MI, const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns) {
+void InstrRefBasedLDV::process(MachineInstr &MI, const FuncValueTable *MLiveOuts,
+ const FuncValueTable *MLiveIns) {
// Try to interpret an MI as a debug or transfer instruction. Only if it's
// none of these should we interpret it's register defs as new value
// definitions.
@@ -3503,7 +3504,10 @@ bool InstrRefBasedLDV::depthFirstVLocAndEmit(
// Helper lambda for ejecting a block -- if nothing is going to use the block,
// we can translate the variable location information into DBG_VALUEs and then
// free all of InstrRefBasedLDV's data structures.
+ SmallPtrSet<const MachineBasicBlock *, 8> EjectedBBs;
auto EjectBlock = [&](MachineBasicBlock &MBB) -> void {
+ if (EjectedBBs.insert(&MBB).second == false)
+ return;
unsigned BBNum = MBB.getNumber();
AllTheVLocs[BBNum].clear();
@@ -3517,14 +3521,14 @@ bool InstrRefBasedLDV::depthFirstVLocAndEmit(
CurBB = BBNum;
CurInst = 1;
for (auto &MI : MBB) {
- process(MI, MOutLocs.get(), MInLocs.get());
+ process(MI, &MOutLocs, &MInLocs);
TTracker->checkInstForNewValues(CurInst, MI.getIterator());
++CurInst;
}
// Free machine-location tables for this block.
- MInLocs[BBNum].reset();
- MOutLocs[BBNum].reset();
+ MInLocs[BBNum] = ValueTable();
+ MOutLocs[BBNum] = ValueTable();
// We don't need live-in variable values for this block either.
Output[BBNum].clear();
AllTheVLocs[BBNum].clear();
@@ -3589,8 +3593,7 @@ bool InstrRefBasedLDV::depthFirstVLocAndEmit(
// anything for such out-of-scope blocks, but for the sake of being similar
// to VarLocBasedLDV, eject these too.
for (auto *MBB : ArtificialBlocks)
- if (MOutLocs[MBB->getNumber()])
- EjectBlock(*MBB);
+ EjectBlock(*MBB);
return emitTransfers(AllVarsNumbering);
}
@@ -3688,14 +3691,9 @@ bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
// Allocate and initialize two array-of-arrays for the live-in and live-out
// machine values. The outer dimension is the block number; while the inner
// dimension is a LocIdx from MLocTracker.
- FuncValueTable MOutLocs = std::make_unique<ValueTable[]>(MaxNumBlocks);
- FuncValueTable MInLocs = std::make_unique<ValueTable[]>(MaxNumBlocks);
unsigned NumLocs = MTracker->getNumLocs();
- for (int i = 0; i < MaxNumBlocks; ++i) {
- // These all auto-initialize to ValueIDNum::EmptyValue
- MOutLocs[i] = std::make_unique<ValueIDNum[]>(NumLocs);
- MInLocs[i] = std::make_unique<ValueIDNum[]>(NumLocs);
- }
+ FuncValueTable MOutLocs(MaxNumBlocks, ValueTable(NumLocs));
+ FuncValueTable MInLocs(MaxNumBlocks, ValueTable(NumLocs));
// Solve the machine value dataflow problem using the MLocTransfer function,
// storing the computed live-ins / live-outs into the array-of-arrays. We use
@@ -3736,7 +3734,7 @@ bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
MTracker->loadFromArray(MInLocs[CurBB], CurBB);
CurInst = 1;
for (auto &MI : MBB) {
- process(MI, MOutLocs.get(), MInLocs.get());
+ process(MI, &MOutLocs, &MInLocs);
++CurInst;
}
MTracker->reset();
@@ -3917,9 +3915,9 @@ class LDVSSAUpdater {
/// Machine location where any PHI must occur.
LocIdx Loc;
/// Table of live-in machine value numbers for blocks / locations.
- const ValueTable *MLiveIns;
+ const FuncValueTable &MLiveIns;
- LDVSSAUpdater(LocIdx L, const ValueTable *MLiveIns)
+ LDVSSAUpdater(LocIdx L, const FuncValueTable &MLiveIns)
: Loc(L), MLiveIns(MLiveIns) {}
void reset() {
@@ -4075,12 +4073,8 @@ template <> class SSAUpdaterTraits<LDVSSAUpdater> {
} // end namespace llvm
std::optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIs(
- MachineFunction &MF, const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns, MachineInstr &Here, uint64_t InstrNum) {
- assert(MLiveOuts && MLiveIns &&
- "Tried to resolve DBG_PHI before location "
- "tables allocated?");
-
+ MachineFunction &MF, const FuncValueTable &MLiveOuts,
+ const FuncValueTable &MLiveIns, MachineInstr &Here, uint64_t InstrNum) {
// This function will be called twice per DBG_INSTR_REF, and might end up
// computing lots of SSA information: memoize it.
auto SeenDbgPHIIt = SeenDbgPHIs.find(std::make_pair(&Here, InstrNum));
@@ -4094,8 +4088,8 @@ std::optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIs(
}
std::optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIsImpl(
- MachineFunction &MF, const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns, MachineInstr &Here, uint64_t InstrNum) {
+ MachineFunction &MF, const FuncValueTable &MLiveOuts,
+ const FuncValueTable &MLiveIns, MachineInstr &Here, uint64_t InstrNum) {
// Pick out records of DBG_PHI instructions that have been observed. If there
// are none, then we cannot compute a value number.
auto RangePair = std::equal_range(DebugPHINumToValue.begin(),
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
index 5e94962f9d7e6..d6dbb1feda3e8 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
@@ -205,11 +205,11 @@ namespace LiveDebugValues {
using namespace llvm;
/// Type for a table of values in a block.
-using ValueTable = std::unique_ptr<ValueIDNum[]>;
+using ValueTable = SmallVector<ValueIDNum, 0>;
/// Type for a table-of-table-of-values, i.e., the collection of either
/// live-in or live-out values for each block in the function.
-using FuncValueTable = std::unique_ptr<ValueTable[]>;
+using FuncValueTable = SmallVector<ValueTable, 0>;
/// Thin wrapper around an integer -- designed to give more type safety to
/// spill location numbers.
@@ -1200,12 +1200,12 @@ class InstrRefBasedLDV : public LDVImpl {
/// exists, otherwise returns std::nullopt.
std::optional<ValueIDNum> getValueForInstrRef(unsigned InstNo, unsigned OpNo,
MachineInstr &MI,
- const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns);
+ const FuncValueTable *MLiveOuts,
+ const FuncValueTable *MLiveIns);
/// Observe a single instruction while stepping through a block.
- void process(MachineInstr &MI, const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns);
+ void process(MachineInstr &MI, const FuncValueTable *MLiveOuts,
+ const FuncValueTable *MLiveIns);
/// Examines whether \p MI is a DBG_VALUE and notifies trackers.
/// \returns true if MI was recognized and processed.
@@ -1213,8 +1213,8 @@ class InstrRefBasedLDV : public LDVImpl {
/// Examines whether \p MI is a DBG_INSTR_REF and notifies trackers.
/// \returns true if MI was recognized and processed.
- bool transferDebugInstrRef(MachineInstr &MI, const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns);
+ bool transferDebugInstrRef(MachineInstr &MI, const FuncValueTable *MLiveOuts,
+ const FuncValueTable *MLiveIns);
/// Stores value-information about where this PHI occurred, and what
/// instruction number is associated with it.
@@ -1246,14 +1246,14 @@ class InstrRefBasedLDV : public LDVImpl {
/// \p InstrNum Debug instruction number defined by DBG_PHI instructions.
/// \returns The machine value number at position Here, or std::nullopt.
std::optional<ValueIDNum> resolveDbgPHIs(MachineFunction &MF,
- const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns,
+ const FuncValueTable &MLiveOuts,
+ const FuncValueTable &MLiveIns,
MachineInstr &Here,
uint64_t InstrNum);
std::optional<ValueIDNum> resolveDbgPHIsImpl(MachineFunction &MF,
- const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns,
+ const FuncValueTable &MLiveOuts,
+ const FuncValueTable &MLiveIns,
MachineInstr &Here,
uint64_t InstrNum);
diff --git a/llvm/unittests/CodeGen/InstrRefLDVTest.cpp b/llvm/unittests/CodeGen/InstrRefLDVTest.cpp
index 5a050bc520362..acbcd247fa9a0 100644
--- a/llvm/unittests/CodeGen/InstrRefLDVTest.cpp
+++ b/llvm/unittests/CodeGen/InstrRefLDVTest.cpp
@@ -497,15 +497,8 @@ body: |
std::pair<FuncValueTable, FuncValueTable>
allocValueTables(unsigned Blocks, unsigned Locs) {
- FuncValueTable MOutLocs = std::make_unique<ValueTable[]>(Blocks);
- FuncValueTable MInLocs = std::make_unique<ValueTable[]>(Blocks);
-
- for (unsigned int I = 0; I < Blocks; ++I) {
- MOutLocs[I] = std::make_unique<ValueIDNum[]>(Locs);
- MInLocs[I] = std::make_unique<ValueIDNum[]>(Locs);
- }
-
- return std::make_pair(std::move(MOutLocs), std::move(MInLocs));
+ return {FuncValueTable(Blocks, ValueTable(Locs)),
+ FuncValueTable(Blocks, ValueTable(Locs))};
}
};
More information about the llvm-commits
mailing list