From: Stephen Tozer Date: Tue, 20 Dec 2022 14:52:11 +0000 (+0000) Subject: [DebugInfo] Unify location selection logic for values in InstrRefBasedImpl X-Git-Tag: upstream/17.0.6~23086 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=a685bb8e333e711d7496f59589694c460f981062;p=platform%2Fupstream%2Fllvm.git [DebugInfo] Unify location selection logic for values in InstrRefBasedImpl Currently the instruction referencing live debug values has 3 separate places where we iterate over all known locations to find the best machine location for a set of values at a given point in the program. Each of these places has an implementation of this check, and one of them has slightly different logic to the others. This patch moves the check for the "quality" of a machine location into a separate function, which also avoids repeatedly calling expensive functions, giving a slight performance improvement. Differential Revision: https://reviews.llvm.org/D140412 --- diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp index 800edf8..fc00b9d 100644 --- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp +++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp @@ -275,7 +275,7 @@ public: ShouldEmitDebugEntryValues = TM.Options.ShouldEmitDebugEntryValues(); } - bool isCalleeSaved(LocIdx L) { + bool isCalleeSaved(LocIdx L) const { unsigned Reg = MTracker->LocIdxToLocID[L]; if (Reg >= MTracker->NumRegs) return false; @@ -285,6 +285,56 @@ public: return false; }; + // An estimate of the expected lifespan of values at a machine location, with + // a greater value corresponding to a longer expected lifespan, i.e. spill + // slots generally live longer than callee-saved registers which generally + // live longer than non-callee-saved registers. The minimum value of 0 + // corresponds to an illegal location that cannot have a "lifespan" at all. + enum class LocationQuality : unsigned char { + Illegal = 0, + Register, + CalleeSavedRegister, + SpillSlot, + Best = SpillSlot + }; + + class LocationAndQuality { + unsigned Location : 24; + unsigned Quality : 8; + + public: + LocationAndQuality() : Location(0), Quality(0) {} + LocationAndQuality(LocIdx L, LocationQuality Q) + : Location(L.asU64()), Quality(static_cast(Q)) {} + LocIdx getLoc() const { + if (!Quality) + return LocIdx::MakeIllegalLoc(); + return LocIdx(Location); + } + LocationQuality getQuality() const { return LocationQuality(Quality); } + bool isIllegal() const { return !Quality; } + bool isBest() const { return getQuality() == LocationQuality::Best; } + }; + + // Returns the LocationQuality for the location L iff the quality of L is + // is strictly greater than the provided minimum quality. + std::optional + getLocQualityIfBetter(LocIdx L, LocationQuality Min) const { + if (L.isIllegal()) + return std::nullopt; + if (Min >= LocationQuality::SpillSlot) + return std::nullopt; + if (MTracker->isSpill(L)) + return LocationQuality::SpillSlot; + if (Min >= LocationQuality::CalleeSavedRegister) + return std::nullopt; + if (isCalleeSaved(L)) + return LocationQuality::CalleeSavedRegister; + if (Min >= LocationQuality::Register) + return std::nullopt; + return LocationQuality::Register; + } + /// For a variable \p Var with the live-in value \p Value, attempts to resolve /// the DbgValue to a concrete DBG_VALUE, emitting that value and loading the /// tracking information to track Var throughout the block. @@ -294,7 +344,7 @@ public: /// \p DbgOpStore is the map containing the DbgOpID->DbgOp mapping needed to /// determine the values used by Value. void loadVarInloc(MachineBasicBlock &MBB, DbgOpIDMap &DbgOpStore, - const DenseMap &ValueToLoc, + const DenseMap &ValueToLoc, DebugVariable Var, DbgValue Value) { SmallVector DbgOps; SmallVector ResolvedDbgOps; @@ -343,7 +393,7 @@ public: // Defer modifying ActiveVLocs until after we've confirmed we have a // live range. - LocIdx M = ValuesPreferredLoc->second; + LocIdx M = ValuesPreferredLoc->second.getLoc(); ResolvedDbgOps.push_back(M); } @@ -390,7 +440,7 @@ public: UseBeforeDefVariables.clear(); // Map of the preferred location for each value. - DenseMap ValueToLoc; + DenseMap ValueToLoc; // Initialized the preferred-location map with illegal locations, to be // filled in later. @@ -398,8 +448,7 @@ public: if (VLoc.second.Kind == DbgValue::Def) for (DbgOpID OpID : VLoc.second.getDbgOpIDs()) if (!OpID.ID.IsConst) - ValueToLoc.insert( - {DbgOpStore.find(OpID).ID, LocIdx::MakeIllegalLoc()}); + ValueToLoc.insert({DbgOpStore.find(OpID).ID, LocationAndQuality()}); ActiveMLocs.reserve(VLocs.size()); ActiveVLocs.reserve(VLocs.size()); @@ -419,16 +468,13 @@ public: if (VIt == ValueToLoc.end()) continue; - LocIdx CurLoc = VIt->second; - // In order of preference, pick: - // * Callee saved registers, - // * Other registers, - // * Spill slots. - if (CurLoc.isIllegal() || MTracker->isSpill(CurLoc) || - (!isCalleeSaved(CurLoc) && isCalleeSaved(Idx.asU64()))) { - // Insert, or overwrite if insertion failed. - VIt->second = Idx; - } + auto &Previous = VIt->second; + // If this is the first location with that value, pick it. Otherwise, + // consider whether it's a "longer term" location. + std::optional ReplacementQuality = + getLocQualityIfBetter(Idx, Previous.getQuality()); + if (ReplacementQuality) + Previous = LocationAndQuality(Idx, *ReplacementQuality); } // Now map variables to their picked LocIdxes. @@ -458,7 +504,7 @@ public: // Map of values to the locations that store them for every value used by // the variables that may have become available. - SmallDenseMap ValueToLoc; + SmallDenseMap ValueToLoc; // Populate ValueToLoc with illegal default mappings for every value used by // any UseBeforeDef variables for this instruction. @@ -472,7 +518,7 @@ public: if (Op.IsConst) continue; - ValueToLoc.insert(std::make_pair(Op.ID, LocIdx::MakeIllegalLoc())); + ValueToLoc.insert({Op.ID, LocationAndQuality()}); } } @@ -490,16 +536,13 @@ public: if (VIt == ValueToLoc.end()) continue; - LocIdx CurLoc = VIt->second; - // In order of preference, pick: - // * Callee saved registers, - // * Other registers, - // * Spill slots. - if (CurLoc.isIllegal() || MTracker->isSpill(CurLoc) || - (!isCalleeSaved(CurLoc) && isCalleeSaved(Idx.asU64()))) { - // Insert, or overwrite if insertion failed. - VIt->second = Idx; - } + auto &Previous = VIt->second; + // If this is the first location with that value, pick it. Otherwise, + // consider whether it's a "longer term" location. + std::optional ReplacementQuality = + getLocQualityIfBetter(Idx, Previous.getQuality()); + if (ReplacementQuality) + Previous = LocationAndQuality(Idx, *ReplacementQuality); } // Using the map of values to locations, produce a final set of values for @@ -515,7 +558,7 @@ public: DbgOps.push_back(Op.MO); continue; } - LocIdx NewLoc = ValueToLoc.find(Op.ID)->second; + LocIdx NewLoc = ValueToLoc.find(Op.ID)->second.getLoc(); if (NewLoc.isIllegal()) break; DbgOps.push_back(NewLoc); @@ -1561,37 +1604,33 @@ bool InstrRefBasedLDV::transferDebugInstrRef(MachineInstr &MI, // Pick a location for the machine value number, if such a location exists. // (This information could be stored in TransferTracker to make it faster). - std::optional FoundLoc; + TransferTracker::LocationAndQuality FoundLoc; for (auto Location : MTracker->locations()) { LocIdx CurL = Location.Idx; ValueIDNum ID = MTracker->readMLoc(CurL); if (NewID && ID == NewID) { // If this is the first location with that value, pick it. Otherwise, // consider whether it's a "longer term" location. - if (!FoundLoc) { - FoundLoc = CurL; - continue; + std::optional ReplacementQuality = + TTracker->getLocQualityIfBetter(CurL, FoundLoc.getQuality()); + if (ReplacementQuality) { + FoundLoc = + TransferTracker::LocationAndQuality(CurL, *ReplacementQuality); + if (FoundLoc.isBest()) + break; } - - if (MTracker->isSpill(CurL)) - FoundLoc = CurL; // Spills are a longer term location. - else if (!MTracker->isSpill(*FoundLoc) && - !MTracker->isSpill(CurL) && - !isCalleeSaved(*FoundLoc) && - isCalleeSaved(CurL)) - FoundLoc = CurL; // Callee saved regs are longer term than normal. } } SmallVector NewLocs; - if (FoundLoc) - NewLocs.push_back(*FoundLoc); + if (!FoundLoc.isIllegal()) + NewLocs.push_back(FoundLoc.getLoc()); // Tell transfer tracker that the variable value has changed. TTracker->redefVar(MI, Properties, NewLocs); // If there was a value with no location; but the value is defined in a // later instruction in this block, this is a block-local use-before-def. - if (!FoundLoc && NewID && NewID->getBlock() == CurBB && + if (FoundLoc.isIllegal() && NewID && NewID->getBlock() == CurBB && NewID->getInst() > CurInst) { SmallVector UseBeforeDefLocs; UseBeforeDefLocs.push_back(*NewID); @@ -1601,7 +1640,7 @@ bool InstrRefBasedLDV::transferDebugInstrRef(MachineInstr &MI, // Produce a DBG_VALUE representing what this DBG_INSTR_REF meant. // This DBG_VALUE is potentially a $noreg / undefined location, if - // FoundLoc is None. + // FoundLoc is illegal. // (XXX -- could morph the DBG_INSTR_REF in the future). MachineInstr *DbgMI = MTracker->emitLoc(NewLocs, V, Properties); diff --git a/llvm/test/DebugInfo/MIR/X86/live-debug-values-restore.mir b/llvm/test/DebugInfo/MIR/X86/live-debug-values-restore.mir index 504735d..9ecf9c1 100644 --- a/llvm/test/DebugInfo/MIR/X86/live-debug-values-restore.mir +++ b/llvm/test/DebugInfo/MIR/X86/live-debug-values-restore.mir @@ -379,8 +379,8 @@ body: | # CHECK-LABEL: name: g # CHECK-NOT: !DIExpression() # CHECK-LABEL: bb.2.if.end: +# CHECK: DBG_VALUE $rbx, $noreg, ![[QVAR]], !DIExpression(DW_OP_LLVM_fragment, 32, 32) # CHECK: DBG_VALUE $rdi, $noreg, ![[QVAR]], !DIExpression(DW_OP_LLVM_fragment, 0, 32) -# CHECK-NEXT: DBG_VALUE $rbx, $noreg, ![[QVAR]], !DIExpression(DW_OP_LLVM_fragment, 32, 32) name: g alignment: 16