Skip to content

[InstrRef][nfc] Remove usage of unique_ptrs of arrays #74203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 14, 2023
Merged

[InstrRef][nfc] Remove usage of unique_ptrs of arrays #74203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23f0c64
[InstrRef][nfc] Remove usage of unique_ptrs of arrays
felipepiovezan Dec 2, 2023
b7f8032
fixup! [InstrRef][nfc] Remove usage of unique_ptrs of arrays
felipepiovezan Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 24 additions & 29 deletions llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -2245,8 +2246,9 @@ 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.
Expand Down Expand Up @@ -3503,7 +3505,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;
Comment on lines +3508 to +3511
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My recollection of this (which may be faulty) is that blocks should only get ejected once, in the order determined just above this block (it reduces the working-set size during compilation). If the added code fires I'd suggest there's a bug somewhere else, is there a reproducer for that? I'd imagine it'd only reduce performance a bit.

Copy link
Contributor Author

@felipepiovezan felipepiovezan Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rationale for this code is because I removed the check on line 3596 of the "green" side of the diff.

I.e., before:

 for (auto *MBB : ArtificialBlocks)
     if (MOutLocs[MBB->getNumber()])
       EjectBlock(*MBB);

After:

  for (auto *MBB : ArtificialBlocks)
     EjectBlock(*MBB);

Prior to this change, we were testing whether something had been ejected by querying whether we had called "reset" on the unique pointer. There are no unique pointers anymore, and checking for whether the length of the vector is 0 not the equivalent, so the only solution was to add this set.

So, with this mind, the patch is 100% intended to be NFC. Do you agree with that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, I see the rationale. I'm struggling to remember where that came from -- I thought it should be the case that the set of ArtificialBlocks and the set of blocks produced by makeDepthFirstEjectionMap shouldn't overlap anyway because the artificial blocks shouldn't be in any lexical scopes? But maybe they can, as LexicalScopes only stores a start/end block-number pair.

Either way, I'm not confident enough to say the is-null test isn't double-ejection prevention so it should be kept. For performance, could you use a SmallDenseMap instead of SmallPtrSet with a call to reserve as we'll know the maximum number of blocks that might get inserted. That limits the worst-case map-growth to a single allocation while providing a local scratchpad for small functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not confident enough to say the is-null test isn't double-ejection prevention so it should be kept

I think there may be some miscommunication going here: the check is kept, it just takes a different form.
I moved it inside the lambda, and instead of checking for a nullptr (pointers don't exist anymore, so we can't check that), I am using an "has been ejected" check.

Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes perfect sense and you're correct; I'm just second-guessing whether the check had a different purpose, or was something I'd gratuitously added in the past without reason.

unsigned BBNum = MBB.getNumber();
AllTheVLocs[BBNum].clear();

Expand All @@ -3517,14 +3522,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();
Expand Down Expand Up @@ -3589,8 +3594,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);
}
Expand Down Expand Up @@ -3688,14 +3692,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
Expand Down Expand Up @@ -3736,7 +3735,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();
Expand Down Expand Up @@ -3917,9 +3916,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() {
Expand Down Expand Up @@ -4075,12 +4074,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));
Expand All @@ -4094,8 +4089,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(),
Expand Down
24 changes: 12 additions & 12 deletions llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(personally, I find code easier to read when it doesn't obscure these outer templates like unique_ptr or SmallVector (ie: I'd rather avoid/remove these aliases and have code refer to SmallVector<ValueTable, 0> directly) - and the name might be a bit misleading too, if we're going to keep it, FuncValueTable - it isn't a single value table, it's a sequence of them - so like ValueTableVector? (at least that hints at it being a sequence, which is helpful - I guess the old name couldn't do that because unique_ptr<T[]> doesn't have begin/end or even a length, etc))

Copy link
Contributor Author

@felipepiovezan felipepiovezan Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.
@jmorse would you be ok if I removed the FuncValueTable typedef and only kept ValueTable?
This would also allow us to pass MutableArrayRefs around instead of SmallVectors

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM!


/// Thin wrapper around an integer -- designed to give more type safety to
/// spill location numbers.
Expand Down Expand Up @@ -1200,21 +1200,21 @@ 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.
bool transferDebugValue(const MachineInstr &MI);

/// 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.
Expand Down Expand Up @@ -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);

Expand Down
11 changes: 2 additions & 9 deletions llvm/unittests/CodeGen/InstrRefLDVTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))};
}
};

Expand Down