[DebugInfo][InstrRef] Index DebugVariables and some DILocations #99318
Conversation
This file contains hidden or 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
A lot of time in LiveDebugValues is spent computing DenseMap keys for DebugVariables, and they're made up of three pointers, so are large. This patch installs an index for them: for the SSA and value-to-location mapping parts of InstrRefBasedLDV we don't need to access things like the variable declaration or the inlining site, so just use a uint32_t identifier for each variable fragment that's tracked. The compile-time performance improvements are substantial (almost 0.4% on the tracker). About 80% of this patch is just replacing DebugVariable references with DebugVariableIDs instead, however there are some larger consequences. We spend lots of time fetching DILocations when emitting DBG_VALUE instructions, so index those with the DebugVariables: this means all DILocations on all DBG_VALUE instructions will normalise to the first-seen DILocation for the variable (which should be fine). We also used to keep an ordering of when each variable was seen first in a DBG_* instruction, in the AllVarsNumbering collection, so that we can emit new DBG_* instructions in a stable order. We can hang this off the DebugVariable index instead, so AllVarsNumbering is deleted. As a consequence of deleting AllVarsNumbering, the order that DBG_* instructions are outputted in can change: we now produce a total order based on RPO exploration of the function, not exploring in normal block-order. That causes the DBG_* instructions in live-debug-values-fragments.mir to change order. These differences don't affect the meaning of the debug-info, but will cause slightly different ordered output, hence this isn't an NFC patch. Downstream tests that fail because of ordering changes /should/ be able to just update the order with no ill effects. Finally, rather than ordering by AllVarsNumbering just before DBG_* instructions are linked into the output MIR, store instructions along with their DebugVariableID, so that they can be sorted by that instead.
|
@llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesA lot of time in LiveDebugValues is spent computing DenseMap keys for DebugVariables, and they're made up of three pointers, so are large. This patch installs an index for them: for the SSA and value-to-location mapping parts of InstrRefBasedLDV we don't need to access things like the variable declaration or the inlining site, so just use a uint32_t identifier for each variable fragment that's tracked. The compile-time performance improvements are substantial (almost 0.4% on the tracker). About 80% of this patch is just replacing DebugVariable references with DebugVariableIDs instead, however there are some larger consequences. We spend lots of time fetching DILocations when emitting DBG_VALUE instructions, so index those with the DebugVariables: this means all DILocations on all new DBG_VALUE instructions will normalise to the first-seen DILocation for the variable (which should be fine). We also used to keep an ordering of when each variable was seen first in a DBG_* instruction, in the AllVarsNumbering collection, so that we can emit new DBG_* instructions in a stable order. We can hang this off the DebugVariable index instead, so AllVarsNumbering is deleted. As a consequence of deleting AllVarsNumbering, the order that DBG_* instructions are outputted in can change: we now produce a total order based on RPO exploration of the function, not exploring in normal block-order. That causes the DBG_* instructions in live-debug-values-fragments.mir to change order. These differences don't affect the meaning of the debug-info, but will cause slightly different ordered output, hence this isn't an NFC patch. Downstream tests that fail because of ordering changes /should/ be able to just update the order with no ill effects. Finally, rather than ordering by AllVarsNumbering just before DBG_* instructions are linked into the output MIR, store instructions along with their DebugVariableID, so that they can be sorted by that instead. (I thought about trying to split this patch up into something smaller, but there's no convenient half-way point without having to generate some dodgy code splicing bits and pieces together, sorry). Patch is 38.51 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/99318.diff 4 Files Affected:
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index 555cbb7a507f4..dbb21344c72d0 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -183,6 +183,7 @@ class TransferTracker {
/// information from it. (XXX make it const?)
MLocTracker *MTracker;
MachineFunction &MF;
+ const DebugVariableMap &DVMap;
bool ShouldEmitDebugEntryValues;
/// Record of all changes in variable locations at a block position. Awkwardly
@@ -191,7 +192,9 @@ class TransferTracker {
struct Transfer {
MachineBasicBlock::instr_iterator Pos; /// Position to insert DBG_VALUes
MachineBasicBlock *MBB; /// non-null if we should insert after.
- SmallVector<MachineInstr *, 4> Insts; /// Vector of DBG_VALUEs to insert.
+ /// Vector of DBG_VALUEs to insert. Store with their DebugVariableID so that
+ /// they can be sorted into a stable order for emission at a later time.
+ SmallVector<std::pair<DebugVariableID, MachineInstr *>, 4> Insts;
};
/// Stores the resolved operands (machine locations and constants) and
@@ -227,15 +230,15 @@ class TransferTracker {
/// Map from LocIdxes to which DebugVariables are based that location.
/// Mantained while stepping through the block. Not accurate if
/// VarLocs[Idx] != MTracker->LocIdxToIDNum[Idx].
- DenseMap<LocIdx, SmallSet<DebugVariable, 4>> ActiveMLocs;
+ DenseMap<LocIdx, SmallSet<DebugVariableID, 4>> ActiveMLocs;
/// Map from DebugVariable to it's current location and qualifying meta
/// information. To be used in conjunction with ActiveMLocs to construct
/// enough information for the DBG_VALUEs for a particular LocIdx.
- DenseMap<DebugVariable, ResolvedDbgValue> ActiveVLocs;
+ DenseMap<DebugVariableID, ResolvedDbgValue> ActiveVLocs;
/// Temporary cache of DBG_VALUEs to be entered into the Transfers collection.
- SmallVector<MachineInstr *, 4> PendingDbgValues;
+ SmallVector<std::pair<DebugVariableID, MachineInstr *>, 4> PendingDbgValues;
/// Record of a use-before-def: created when a value that's live-in to the
/// current block isn't available in any machine location, but it will be
@@ -244,12 +247,12 @@ class TransferTracker {
/// Value of this variable, def'd in block.
SmallVector<DbgOp> Values;
/// Identity of this variable.
- DebugVariable Var;
+ DebugVariableID VarID;
/// Additional variable properties.
DbgValueProperties Properties;
- UseBeforeDef(ArrayRef<DbgOp> Values, const DebugVariable &Var,
+ UseBeforeDef(ArrayRef<DbgOp> Values, DebugVariableID VarID,
const DbgValueProperties &Properties)
- : Values(Values.begin(), Values.end()), Var(Var),
+ : Values(Values.begin(), Values.end()), VarID(VarID),
Properties(Properties) {}
};
@@ -260,15 +263,16 @@ class TransferTracker {
/// The set of variables that are in UseBeforeDefs and can become a location
/// once the relevant value is defined. An element being erased from this
/// collection prevents the use-before-def materializing.
- DenseSet<DebugVariable> UseBeforeDefVariables;
+ DenseSet<DebugVariableID> UseBeforeDefVariables;
const TargetRegisterInfo &TRI;
const BitVector &CalleeSavedRegs;
TransferTracker(const TargetInstrInfo *TII, MLocTracker *MTracker,
- MachineFunction &MF, const TargetRegisterInfo &TRI,
+ MachineFunction &MF, const DebugVariableMap &DVMap,
+ const TargetRegisterInfo &TRI,
const BitVector &CalleeSavedRegs, const TargetPassConfig &TPC)
- : TII(TII), MTracker(MTracker), MF(MF), TRI(TRI),
+ : TII(TII), MTracker(MTracker), MF(MF), DVMap(DVMap), TRI(TRI),
CalleeSavedRegs(CalleeSavedRegs) {
TLI = MF.getSubtarget().getTargetLowering();
auto &TM = TPC.getTM<TargetMachine>();
@@ -345,7 +349,7 @@ class TransferTracker {
/// determine the values used by Value.
void loadVarInloc(MachineBasicBlock &MBB, DbgOpIDMap &DbgOpStore,
const DenseMap<ValueIDNum, LocationAndQuality> &ValueToLoc,
- DebugVariable Var, DbgValue Value) {
+ DebugVariableID VarID, DbgValue Value) {
SmallVector<DbgOp> DbgOps;
SmallVector<ResolvedDbgOp> ResolvedDbgOps;
bool IsValueValid = true;
@@ -386,7 +390,7 @@ class TransferTracker {
static_cast<unsigned>(Num.getInst()));
continue;
}
- recoverAsEntryValue(Var, Value.Properties, Num);
+ recoverAsEntryValue(VarID, Value.Properties, Num);
IsValueValid = false;
break;
}
@@ -404,8 +408,7 @@ class TransferTracker {
// Add UseBeforeDef entry for the last value to be defined in this block.
if (LastUseBeforeDef) {
- addUseBeforeDef(Var, Value.Properties, DbgOps,
- LastUseBeforeDef);
+ addUseBeforeDef(VarID, Value.Properties, DbgOps, LastUseBeforeDef);
return;
}
@@ -413,13 +416,15 @@ class TransferTracker {
// the transfer.
for (const ResolvedDbgOp &Op : ResolvedDbgOps)
if (!Op.IsConst)
- ActiveMLocs[Op.Loc].insert(Var);
+ ActiveMLocs[Op.Loc].insert(VarID);
auto NewValue = ResolvedDbgValue{ResolvedDbgOps, Value.Properties};
- auto Result = ActiveVLocs.insert(std::make_pair(Var, NewValue));
+ auto Result = ActiveVLocs.insert(std::make_pair(VarID, NewValue));
if (!Result.second)
Result.first->second = NewValue;
+ auto &[Var, DILoc] = DVMap.lookupDVID(VarID);
PendingDbgValues.push_back(
- MTracker->emitLoc(ResolvedDbgOps, Var, Value.Properties));
+ std::make_pair(VarID, &*MTracker->emitLoc(ResolvedDbgOps, Var, DILoc,
+ Value.Properties)));
}
/// Load object with live-in variable values. \p mlocs contains the live-in
@@ -430,7 +435,7 @@ class TransferTracker {
/// FIXME: could just examine mloctracker instead of passing in \p mlocs?
void
loadInlocs(MachineBasicBlock &MBB, ValueTable &MLocs, DbgOpIDMap &DbgOpStore,
- const SmallVectorImpl<std::pair<DebugVariable, DbgValue>> &VLocs,
+ const SmallVectorImpl<std::pair<DebugVariableID, DbgValue>> &VLocs,
unsigned NumLocs) {
ActiveMLocs.clear();
ActiveVLocs.clear();
@@ -486,11 +491,11 @@ class TransferTracker {
/// Record that \p Var has value \p ID, a value that becomes available
/// later in the function.
- void addUseBeforeDef(const DebugVariable &Var,
+ void addUseBeforeDef(DebugVariableID VarID,
const DbgValueProperties &Properties,
const SmallVectorImpl<DbgOp> &DbgOps, unsigned Inst) {
- UseBeforeDefs[Inst].emplace_back(DbgOps, Var, Properties);
- UseBeforeDefVariables.insert(Var);
+ UseBeforeDefs[Inst].emplace_back(DbgOps, VarID, Properties);
+ UseBeforeDefVariables.insert(VarID);
}
/// After the instruction at index \p Inst and position \p pos has been
@@ -509,7 +514,7 @@ class TransferTracker {
// Populate ValueToLoc with illegal default mappings for every value used by
// any UseBeforeDef variables for this instruction.
for (auto &Use : MIt->second) {
- if (!UseBeforeDefVariables.count(Use.Var))
+ if (!UseBeforeDefVariables.count(Use.VarID))
continue;
for (DbgOp &Op : Use.Values) {
@@ -548,7 +553,7 @@ class TransferTracker {
// Using the map of values to locations, produce a final set of values for
// this variable.
for (auto &Use : MIt->second) {
- if (!UseBeforeDefVariables.count(Use.Var))
+ if (!UseBeforeDefVariables.count(Use.VarID))
continue;
SmallVector<ResolvedDbgOp> DbgOps;
@@ -571,8 +576,9 @@ class TransferTracker {
continue;
// Otherwise, we're good to go.
- PendingDbgValues.push_back(
- MTracker->emitLoc(DbgOps, Use.Var, Use.Properties));
+ auto &[Var, DILoc] = DVMap.lookupDVID(Use.VarID);
+ PendingDbgValues.push_back(std::make_pair(
+ Use.VarID, MTracker->emitLoc(DbgOps, Var, DILoc, Use.Properties)));
}
flushDbgValues(pos, nullptr);
}
@@ -622,7 +628,7 @@ class TransferTracker {
return Reg != SP && Reg != FP;
}
- bool recoverAsEntryValue(const DebugVariable &Var,
+ bool recoverAsEntryValue(DebugVariableID VarID,
const DbgValueProperties &Prop,
const ValueIDNum &Num) {
// Is this variable location a candidate to be an entry value. First,
@@ -643,6 +649,8 @@ class TransferTracker {
DIExpr = *NonVariadicExpression;
}
+ auto &[Var, DILoc] = DVMap.lookupDVID(VarID);
+
// Is the variable appropriate for entry values (i.e., is a parameter).
if (!isEntryValueVariable(Var, DIExpr))
return false;
@@ -656,9 +664,8 @@ class TransferTracker {
DIExpression::prepend(DIExpr, DIExpression::EntryValue);
Register Reg = MTracker->LocIdxToLocID[Num.getLoc()];
MachineOperand MO = MachineOperand::CreateReg(Reg, false);
-
- PendingDbgValues.push_back(
- emitMOLoc(MO, Var, {NewExpr, Prop.Indirect, false}));
+ PendingDbgValues.push_back(std::make_pair(
+ VarID, &*emitMOLoc(MO, Var, {NewExpr, Prop.Indirect, false})));
return true;
}
@@ -667,19 +674,20 @@ class TransferTracker {
DebugVariable Var(MI.getDebugVariable(), MI.getDebugExpression(),
MI.getDebugLoc()->getInlinedAt());
DbgValueProperties Properties(MI);
+ DebugVariableID VarID = DVMap.getDVID(Var);
// Ignore non-register locations, we don't transfer those.
if (MI.isUndefDebugValue() ||
all_of(MI.debug_operands(),
[](const MachineOperand &MO) { return !MO.isReg(); })) {
- auto It = ActiveVLocs.find(Var);
+ auto It = ActiveVLocs.find(VarID);
if (It != ActiveVLocs.end()) {
for (LocIdx Loc : It->second.loc_indices())
- ActiveMLocs[Loc].erase(Var);
+ ActiveMLocs[Loc].erase(VarID);
ActiveVLocs.erase(It);
}
// Any use-before-defs no longer apply.
- UseBeforeDefVariables.erase(Var);
+ UseBeforeDefVariables.erase(VarID);
return;
}
@@ -705,14 +713,15 @@ class TransferTracker {
SmallVectorImpl<ResolvedDbgOp> &NewLocs) {
DebugVariable Var(MI.getDebugVariable(), MI.getDebugExpression(),
MI.getDebugLoc()->getInlinedAt());
+ DebugVariableID VarID = DVMap.getDVID(Var);
// Any use-before-defs no longer apply.
- UseBeforeDefVariables.erase(Var);
+ UseBeforeDefVariables.erase(VarID);
// Erase any previous location.
- auto It = ActiveVLocs.find(Var);
+ auto It = ActiveVLocs.find(VarID);
if (It != ActiveVLocs.end()) {
for (LocIdx Loc : It->second.loc_indices())
- ActiveMLocs[Loc].erase(Var);
+ ActiveMLocs[Loc].erase(VarID);
}
// If there _is_ no new location, all we had to do was erase.
@@ -722,7 +731,7 @@ class TransferTracker {
return;
}
- SmallVector<std::pair<LocIdx, DebugVariable>> LostMLocs;
+ SmallVector<std::pair<LocIdx, DebugVariableID>> LostMLocs;
for (ResolvedDbgOp &Op : NewLocs) {
if (Op.IsConst)
continue;
@@ -749,17 +758,17 @@ class TransferTracker {
for (const auto &LostMLoc : LostMLocs)
ActiveMLocs[LostMLoc.first].erase(LostMLoc.second);
LostMLocs.clear();
- It = ActiveVLocs.find(Var);
+ It = ActiveVLocs.find(VarID);
ActiveMLocs[NewLoc.asU64()].clear();
VarLocs[NewLoc.asU64()] = MTracker->readMLoc(NewLoc);
}
- ActiveMLocs[NewLoc].insert(Var);
+ ActiveMLocs[NewLoc].insert(VarID);
}
if (It == ActiveVLocs.end()) {
ActiveVLocs.insert(
- std::make_pair(Var, ResolvedDbgValue(NewLocs, Properties)));
+ std::make_pair(VarID, ResolvedDbgValue(NewLocs, Properties)));
} else {
It->second.Ops.assign(NewLocs);
It->second.Properties = Properties;
@@ -802,21 +811,21 @@ class TransferTracker {
// explicitly undef, then stop here.
if (!NewLoc && !MakeUndef) {
// Try and recover a few more locations with entry values.
- for (const auto &Var : ActiveMLocIt->second) {
- auto &Prop = ActiveVLocs.find(Var)->second.Properties;
- recoverAsEntryValue(Var, Prop, OldValue);
+ for (DebugVariableID VarID : ActiveMLocIt->second) {
+ auto &Prop = ActiveVLocs.find(VarID)->second.Properties;
+ recoverAsEntryValue(VarID, Prop, OldValue);
}
flushDbgValues(Pos, nullptr);
return;
}
// Examine all the variables based on this location.
- DenseSet<DebugVariable> NewMLocs;
+ DenseSet<DebugVariableID> NewMLocs;
// If no new location has been found, every variable that depends on this
// MLoc is dead, so end their existing MLoc->Var mappings as well.
- SmallVector<std::pair<LocIdx, DebugVariable>> LostMLocs;
- for (const auto &Var : ActiveMLocIt->second) {
- auto ActiveVLocIt = ActiveVLocs.find(Var);
+ SmallVector<std::pair<LocIdx, DebugVariableID>> LostMLocs;
+ for (DebugVariableID VarID : ActiveMLocIt->second) {
+ auto ActiveVLocIt = ActiveVLocs.find(VarID);
// Re-state the variable location: if there's no replacement then NewLoc
// is std::nullopt and a $noreg DBG_VALUE will be created. Otherwise, a
// DBG_VALUE identifying the alternative location will be emitted.
@@ -835,19 +844,21 @@ class TransferTracker {
replace_copy(ActiveVLocIt->second.Ops, DbgOps.begin(), OldOp, NewOp);
}
- PendingDbgValues.push_back(MTracker->emitLoc(DbgOps, Var, Properties));
+ auto &[Var, DILoc] = DVMap.lookupDVID(VarID);
+ PendingDbgValues.push_back(std::make_pair(
+ VarID, &*MTracker->emitLoc(DbgOps, Var, DILoc, Properties)));
// Update machine locations <=> variable locations maps. Defer updating
// ActiveMLocs to avoid invalidating the ActiveMLocIt iterator.
if (!NewLoc) {
for (LocIdx Loc : ActiveVLocIt->second.loc_indices()) {
if (Loc != MLoc)
- LostMLocs.emplace_back(Loc, Var);
+ LostMLocs.emplace_back(Loc, VarID);
}
ActiveVLocs.erase(ActiveVLocIt);
} else {
ActiveVLocIt->second.Ops = DbgOps;
- NewMLocs.insert(Var);
+ NewMLocs.insert(VarID);
}
}
@@ -871,8 +882,8 @@ class TransferTracker {
// Commit ActiveMLoc changes.
ActiveMLocIt->second.clear();
if (!NewMLocs.empty())
- for (auto &Var : NewMLocs)
- ActiveMLocs[*NewLoc].insert(Var);
+ for (DebugVariableID VarID : NewMLocs)
+ ActiveMLocs[*NewLoc].insert(VarID);
}
/// Transfer variables based on \p Src to be based on \p Dst. This handles
@@ -895,17 +906,18 @@ class TransferTracker {
// For each variable based on Src; create a location at Dst.
ResolvedDbgOp SrcOp(Src);
ResolvedDbgOp DstOp(Dst);
- for (const auto &Var : MovingVars) {
- auto ActiveVLocIt = ActiveVLocs.find(Var);
+ for (DebugVariableID VarID : MovingVars) {
+ auto ActiveVLocIt = ActiveVLocs.find(VarID);
assert(ActiveVLocIt != ActiveVLocs.end());
// Update all instances of Src in the variable's tracked values to Dst.
std::replace(ActiveVLocIt->second.Ops.begin(),
ActiveVLocIt->second.Ops.end(), SrcOp, DstOp);
- MachineInstr *MI = MTracker->emitLoc(ActiveVLocIt->second.Ops, Var,
+ auto &[Var, DILoc] = DVMap.lookupDVID(VarID);
+ MachineInstr *MI = MTracker->emitLoc(ActiveVLocIt->second.Ops, Var, DILoc,
ActiveVLocIt->second.Properties);
- PendingDbgValues.push_back(MI);
+ PendingDbgValues.push_back(std::make_pair(VarID, MI));
}
ActiveMLocs[Src].clear();
flushDbgValues(Pos, nullptr);
@@ -1156,11 +1168,9 @@ LLVM_DUMP_METHOD void MLocTracker::dump_mloc_map() {
MachineInstrBuilder
MLocTracker::emitLoc(const SmallVectorImpl<ResolvedDbgOp> &DbgOps,
- const DebugVariable &Var,
+ const DebugVariable &Var, const DILocation *DILoc,
const DbgValueProperties &Properties) {
- DebugLoc DL = DILocation::get(Var.getVariable()->getContext(), 0, 0,
- Var.getVariable()->getScope(),
- const_cast<DILocation *>(Var.getInlinedAt()));
+ DebugLoc DL = DebugLoc(DILoc);
const MCInstrDesc &Desc = Properties.IsVariadic
? TII.get(TargetOpcode::DBG_VALUE_LIST)
@@ -1706,7 +1716,8 @@ bool InstrRefBasedLDV::transferDebugInstrRef(MachineInstr &MI,
LastUseBeforeDef = std::max(LastUseBeforeDef, NewID.getInst());
}
if (IsValidUseBeforeDef) {
- TTracker->addUseBeforeDef(V, {MI.getDebugExpression(), false, true},
+ DebugVariableID VID = DVMap.insertDVID(V, MI.getDebugLoc().get());
+ TTracker->addUseBeforeDef(VID, {MI.getDebugExpression(), false, true},
DbgOps, LastUseBeforeDef);
}
}
@@ -1715,9 +1726,11 @@ bool InstrRefBasedLDV::transferDebugInstrRef(MachineInstr &MI,
// This DBG_VALUE is potentially a $noreg / undefined location, if
// FoundLoc is illegal.
// (XXX -- could morph the DBG_INSTR_REF in the future).
- MachineInstr *DbgMI = MTracker->emitLoc(NewLocs, V, Properties);
+ MachineInstr *DbgMI =
+ MTracker->emitLoc(NewLocs, V, MI.getDebugLoc().get(), Properties);
+ DebugVariableID ID = DVMap.getDVID(V);
- TTracker->PendingDbgValues.push_back(DbgMI);
+ TTracker->PendingDbgValues.push_back(std::make_pair(ID, DbgMI));
TTracker->flushDbgValues(MI.getIterator(), nullptr);
return true;
}
@@ -3092,7 +3105,8 @@ void InstrRefBasedLDV::getBlocksForScope(
}
void InstrRefBasedLDV::buildVLocValueMap(
- const DILocation *DILoc, const SmallSet<DebugVariable, 4> &VarsWeCareAbout,
+ const DILocation *DILoc,
+ const SmallSet<DebugVariableID, 4> &VarsWeCareAbout,
SmallPtrSetImpl<MachineBasicBlock *> &AssignBlocks, LiveInsT &Output,
FuncValueTable &MOutLocs, FuncValueTable &MInLocs,
SmallVectorImpl<VLocTracker> &AllTheVLocs) {
@@ -3167,7 +3181,7 @@ void InstrRefBasedLDV::buildVLocValueMap(
// between blocks. This keeps the locality of working on one lexical scope at
// at time, but avoids re-processing variable values because some other
// variable has been assigned.
- for (const auto &Var : VarsWeCareAbout) {
+ for (DebugVariableID VarID : VarsWeCareAbout) {
// Re-initialize live-ins and live-outs, to clear the remains of previous
// variables live-ins / live-outs.
for (unsigned int I = 0; I < NumBlocks; ++I) {
@@ -3181,7 +3195,7 @@ void InstrRefBasedLDV::buildVLocValueMap(
SmallPtrSet<MachineBasicBlock *, 32> DefBlocks;
for (const MachineBasicBlock *ExpMBB : BlocksToExplore) {
auto &TransferFunc = AllTheVLocs[ExpMBB->getNumber()].Vars;
- if (TransferFunc.contains(Var))
+ if (TransferFunc.contains(VarID))
DefBlocks.insert(const_cast<MachineBasicBlock *>(ExpMBB));
}
@@ -3191,7 +3205,7 @@ void InstrRefBasedLDV::buildVLocValueMap(
// only one value definition, things are very simple.
if (DefBlocks.size() == 1) {
placePHIsForSingleVarDefinition(MutBlocksToExplore, *DefBlocks.begin(),
- AllTheVLocs, Var, Output);
+ AllTheVLocs, VarID, Output);
continue;
}
@@ -3264,7 +3278,7 @@ void InstrRefBasedLDV::buildVLocValueMap(
// Do transfer function.
auto &VTracker = AllTheVLocs[MBB->getNumber()];
- auto TransferIt = VTracker.Vars.find(Var);
+ auto TransferIt = VTracker.Vars.find(VarID);
if (TransferIt != VTracker.Vars.end()) {
// Erase on empty transfer (DBG_VALUE $noreg).
if (TransferIt->second.Kind == DbgValue::Undef) {
@@ -3326,9 +3340,11 @@ void InstrRefBasedLDV::buildVLocValueMap(
continue;
if (BlockLiveIn->Kind == DbgValue::VPHI)
BlockLiveIn->Kind = DbgValue::Def;
+ auto &[Var, DILoc] = DVMap.lookupDVID(VarID);
assert(BlockLiveIn->Properties.DIExpr->getFragmentInfo()...
[truncated]
|
OCHyams
approved these changes
Jul 17, 2024
| /// Map DebugVariable to the latest Value it's defined to have. | ||
| /// Needs to be a MapVector because we determine order-in-the-input-MIR from | ||
| /// the order in this container. | ||
| /// the order in this container. (FIXME: this is less true now). |
There was a problem hiding this comment.
Imo it could be helpful to future developers if you explained why it's less true now here?
…bles-pls Conflicts: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp Combination of the ordering-of-a-vector change with the DebugVaraibleID change
|
/me squints -- so after merging with 2 weeks of master I see the test-change has undone itself. Furthermore, my earlier analysis about it changing to RPO order is wrong because that test function (foo) contains straight line code. Not sure what happened there, but it's even more correct now. |
|
Ah, actually, it's going to be because of obscure dependencies between the other speedup patches I've got -- they were all one massive diff, which I've separated into reviewable patches. Panic over. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/1878 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/2608 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/1688 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/1894 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/3403 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/1935 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/2729 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/1969 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/8/builds/1768 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/108/builds/1409 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/2502 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/1868 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/1944 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/1893 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/2417 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/2106 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/1128 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/133/builds/1789 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/12/builds/2141 Here is the relevant piece of the build log for the reference: |
|
Oh yeah. The unit tests. That I've built up a habit of ignoring while iterating. Whoops. |
jmorse
added a commit
that referenced
this pull request
Jul 18, 2024
Now revised to actually make the unit test compile, which I'd been ignoring. No actual functional change, it's a type difference. Original commit message follows. [DebugInfo][InstrRef] Index DebugVariables and some DILocations (#99318) A lot of time in LiveDebugValues is spent computing DenseMap keys for DebugVariables, and they're made up of three pointers, so are large. This patch installs an index for them: for the SSA and value-to-location mapping parts of InstrRefBasedLDV we don't need to access things like the variable declaration or the inlining site, so just use a uint32_t identifier for each variable fragment that's tracked. The compile-time performance improvements are substantial (almost 0.4% on the tracker). About 80% of this patch is just replacing DebugVariable references with DebugVariableIDs instead, however there are some larger consequences. We spend lots of time fetching DILocations when emitting DBG_VALUE instructions, so index those with the DebugVariables: this means all DILocations on all new DBG_VALUE instructions will normalise to the first-seen DILocation for the variable (which should be fine). We also used to keep an ordering of when each variable was seen first in a DBG_* instruction, in the AllVarsNumbering collection, so that we can emit new DBG_* instructions in a stable order. We can hang this off the DebugVariable index instead, so AllVarsNumbering is deleted. Finally, rather than ordering by AllVarsNumbering just before DBG_* instructions are linked into the output MIR, store instructions along with their DebugVariableID, so that they can be sorted by that instead.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/959 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/1980 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/2816 Here is the relevant piece of the build log for the reference: |
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
Summary: A lot of time in LiveDebugValues is spent computing DenseMap keys for DebugVariables, and they're made up of three pointers, so are large. This patch installs an index for them: for the SSA and value-to-location mapping parts of InstrRefBasedLDV we don't need to access things like the variable declaration or the inlining site, so just use a uint32_t identifier for each variable fragment that's tracked. The compile-time performance improvements are substantial (almost 0.4% on the tracker). About 80% of this patch is just replacing DebugVariable references with DebugVariableIDs instead, however there are some larger consequences. We spend lots of time fetching DILocations when emitting DBG_VALUE instructions, so index those with the DebugVariables: this means all DILocations on all new DBG_VALUE instructions will normalise to the first-seen DILocation for the variable (which should be fine). We also used to keep an ordering of when each variable was seen first in a DBG_* instruction, in the AllVarsNumbering collection, so that we can emit new DBG_* instructions in a stable order. We can hang this off the DebugVariable index instead, so AllVarsNumbering is deleted. Finally, rather than ordering by AllVarsNumbering just before DBG_* instructions are linked into the output MIR, store instructions along with their DebugVariableID, so that they can be sorted by that instead. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250779
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
Now revised to actually make the unit test compile, which I'd been ignoring. No actual functional change, it's a type difference. Original commit message follows. [DebugInfo][InstrRef] Index DebugVariables and some DILocations (#99318) A lot of time in LiveDebugValues is spent computing DenseMap keys for DebugVariables, and they're made up of three pointers, so are large. This patch installs an index for them: for the SSA and value-to-location mapping parts of InstrRefBasedLDV we don't need to access things like the variable declaration or the inlining site, so just use a uint32_t identifier for each variable fragment that's tracked. The compile-time performance improvements are substantial (almost 0.4% on the tracker). About 80% of this patch is just replacing DebugVariable references with DebugVariableIDs instead, however there are some larger consequences. We spend lots of time fetching DILocations when emitting DBG_VALUE instructions, so index those with the DebugVariables: this means all DILocations on all new DBG_VALUE instructions will normalise to the first-seen DILocation for the variable (which should be fine). We also used to keep an ordering of when each variable was seen first in a DBG_* instruction, in the AllVarsNumbering collection, so that we can emit new DBG_* instructions in a stable order. We can hang this off the DebugVariable index instead, so AllVarsNumbering is deleted. Finally, rather than ordering by AllVarsNumbering just before DBG_* instructions are linked into the output MIR, store instructions along with their DebugVariableID, so that they can be sorted by that instead.
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.
A lot of time in LiveDebugValues is spent computing DenseMap keys for DebugVariables, and they're made up of three pointers, so are large. This patch installs an index for them: for the SSA and value-to-location mapping parts of InstrRefBasedLDV we don't need to access things like the variable declaration or the inlining site, so just use a uint32_t identifier for each variable fragment that's tracked. The compile-time performance improvements are substantial (almost 0.4% on the tracker).
About 80% of this patch is just replacing DebugVariable references with DebugVariableIDs instead, however there are some larger consequences. We spend lots of time fetching DILocations when emitting DBG_VALUE instructions, so index those with the DebugVariables: this means all DILocations on all new DBG_VALUE instructions will normalise to the first-seen DILocation for the variable (which should be fine).
We also used to keep an ordering of when each variable was seen first in a DBG_* instruction, in the AllVarsNumbering collection, so that we can emit new DBG_* instructions in a stable order. We can hang this off the DebugVariable index instead, so AllVarsNumbering is deleted.
As a consequence of deleting AllVarsNumbering, the order that DBG_* instructions are outputted in can change: we now produce a total order based on RPO exploration of the function, not exploring in normal block-order. That causes the DBG_* instructions in live-debug-values-fragments.mir to change order. These differences don't affect the meaning of the debug-info, but will cause slightly different ordered output, hence this isn't an NFC patch. Downstream tests that fail because of ordering changes /should/ be able to just update the order with no ill effects.
Finally, rather than ordering by AllVarsNumbering just before DBG_* instructions are linked into the output MIR, store instructions along with their DebugVariableID, so that they can be sorted by that instead.
(I thought about trying to split this patch up into something smaller, but there's no convenient half-way point without having to generate some dodgy code splicing bits and pieces together, sorry).