Address review comments, split out value-name mapping to separate class, remove line tracking

Author: SLTozer

llvm/include/llvm/Transforms/Utils/DebugSSAUpdater.h

@@ -13,24 +13,30 @@
 // the state of debug variables, such as measuring the change in values of a
 // variable over time, or calculating coverage stats.
 //
+// NB: This is an expensive analysis that is generally not suitable for use in
+// LLVM passes, but may be useful for standalone tools.
+//
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TRANSFORMS_UTILS_DEBUGSSAUPDATER_H
 #define LLVM_TRANSFORMS_UTILS_DEBUGSSAUPDATER_H
 
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DebugProgramInstruction.h"
 #include "llvm/IR/Instruction.h"
+#include "llvm/IR/ValueHandle.h"
+#include "llvm/IR/ValueMap.h"
+#include <cstdint>
 
 namespace llvm {
 
 ////////////////////////////////////////
 // SSAUpdater specialization classes
 
 class DbgSSAPhi;
-template <typename T> class SmallVectorImpl;
 template <typename T> class SSAUpdaterTraits;
 
 /// A definition of a variable; can represent either a debug value, no
@@ -208,12 +214,15 @@ class DbgSSABlock {
 /// SSA construction (via the SSAUpdaterImpl class), used for analysis purposes.
 class DebugSSAUpdater {
   friend class SSAUpdaterTraits<DebugSSAUpdater>;
+  using AvailableValsTy = DenseMap<DbgSSABlock *, DbgValueDef>;
 
 private:
   /// This keeps track of which value to use on a per-block basis. When we
   /// insert PHI nodes, we keep track of them here.
-  void *AV = nullptr;
+  AvailableValsTy AV;
 
+  /// Pointer to an optionally-passed vector into which, if it is non-null,
+  /// the PHIs that describe ambiguous variable locations will be inserted.
   SmallVectorImpl<DbgSSAPhi *> *InsertedPHIs;
 
   DenseMap<BasicBlock *, DbgSSABlock *> BlockMap;
@@ -225,7 +234,6 @@ class DebugSSAUpdater {
       SmallVectorImpl<DbgSSAPhi *> *InsertedPHIs = nullptr);
   DebugSSAUpdater(const DebugSSAUpdater &) = delete;
   DebugSSAUpdater &operator=(const DebugSSAUpdater &) = delete;
-  ~DebugSSAUpdater();
 
   void reset() {
     for (auto &Block : BlockMap)
@@ -298,21 +306,35 @@ struct DbgRangeEntry {
   DbgValueDef Value;
 };
 
+/// Utility class used to store the names of SSA values after their owning
+/// modules have been destroyed. Values are added via \c addValue to receive a
+/// corresponding ID, which can then be used to retrieve the name of the SSA
+/// value via \c getName at any point. Adding the same value multiple times
+/// returns the same ID, making \c addValue idempotent.
+class SSAValueNameMap {
+  struct Config : ValueMapConfig<Value *> {
+    enum { FollowRAUW = false };
+  };
+public:
+  using ValueID = uint64_t;
+  ValueID addValue(Value *V);
+  std::string getName(ValueID ID) {
+    return ValueIDToNameMap[ID];
+  }
+private:
+  DenseMap<ValueID, std::string> ValueIDToNameMap;
+  ValueMap<Value*, ValueID, Config> ValueToIDMap;
+  ValueID NextID = 0;
+};
+
+/// Utility class used to find and store the live debug ranges for variables in
+/// a module. This class uses the DebugSSAUpdater for each variable added with
+/// \c addVariable to find either a single-location value, e.g. #dbg_declare, or
+/// a set of live value ranges corresponding to the set of #dbg_value records.
 class DbgValueRangeTable {
   DenseMap<DebugVariableAggregate, SmallVector<DbgRangeEntry>>
       OrigVariableValueRangeTable;
   DenseMap<DebugVariableAggregate, DbgValueDef> OrigSingleLocVariableValueTable;
-  // For the only initial user of this class, the mappings below are useful and
-  // are used in conjunction with the variable value ranges above, thus we track
-  // them as part of the same class. If we have more uses for variable value
-  // range tracking, then the line/variable name mapping should be moved out to
-  // a separate class.
-  DenseMap<BasicBlock::iterator, uint64_t> LineMapping;
-  DenseMap<uint64_t, std::string> VariableNameMapping;
-
-  using VarMapping = DenseMap<Value *, uint64_t>;
-  VarMapping VariableMapping;
-  uint64_t KeyIndex = 0;
 
 public:
   void addVariable(Function *F, DebugVariableAggregate DVA);
@@ -330,19 +352,6 @@ class DbgValueRangeTable {
     return OrigSingleLocVariableValueTable[DVA];
   }
 
-  void addLine(BasicBlock::iterator I, uint64_t LineAddr) {
-    LineMapping[I] = LineAddr;
-  }
-  uint64_t getLine(BasicBlock::iterator I) {
-    return LineMapping.contains(I) ? LineMapping[I] : (uint64_t)-1;
-  }
-
-  uint64_t addVariableName(Value *V, uint64_t Size);
-  std::string getVariableName(uint64_t Key) {
-    assert(VariableNameMapping.contains(Key) && "Why not here?");
-    return VariableNameMapping[Key];
-  }
-
   void printValues(DebugVariableAggregate DVA, raw_ostream &OS);
 };
 

llvm/lib/Transforms/Utils/DebugSSAUpdater.cpp

@@ -42,34 +42,23 @@ void DbgSSAPhi::print(raw_ostream &OS) const {
 
 using AvailableValsTy = DenseMap<DbgSSABlock *, DbgValueDef>;
 
-static AvailableValsTy &getAvailableVals(void *AV) {
-  return *static_cast<AvailableValsTy *>(AV);
-}
-
 DebugSSAUpdater::DebugSSAUpdater(SmallVectorImpl<DbgSSAPhi *> *NewPHI)
     : InsertedPHIs(NewPHI) {}
 
-DebugSSAUpdater::~DebugSSAUpdater() {
-  delete static_cast<AvailableValsTy *>(AV);
-}
-
 void DebugSSAUpdater::initialize() {
-  if (!AV)
-    AV = new AvailableValsTy();
-  else
-    getAvailableVals(AV).clear();
+  AV.clear();
 }
 
 bool DebugSSAUpdater::hasValueForBlock(DbgSSABlock *BB) const {
-  return getAvailableVals(AV).count(BB);
+  return AV.count(BB);
 }
 
 DbgValueDef DebugSSAUpdater::findValueForBlock(DbgSSABlock *BB) const {
-  return getAvailableVals(AV).lookup(BB);
+  return AV.lookup(BB);
 }
 
 void DebugSSAUpdater::addAvailableValue(DbgSSABlock *BB, DbgValueDef DV) {
-  getAvailableVals(AV)[BB] = DV;
+  AV[BB] = DV;
 }
 
 DbgValueDef DebugSSAUpdater::getValueAtEndOfBlock(DbgSSABlock *BB) {
@@ -171,8 +160,8 @@ template <> class SSAUpdaterTraits<DebugSSAUpdater> {
   static PHI_iterator PHI_begin(PhiT *PHI) { return PHI_iterator(PHI); }
   static PHI_iterator PHI_end(PhiT *PHI) { return PHI_iterator(PHI, true); }
 
-  /// FindPredecessorBlocks - Put the predecessors of Info->BB into the Preds
-  /// vector, set Info->NumPreds, and allocate space in Info->Preds.
+  /// FindPredecessorBlocks - Put the predecessors of BB into the Preds
+  /// vector.
   static void FindPredecessorBlocks(DbgSSABlock *BB,
                                     SmallVectorImpl<DbgSSABlock *> *Preds) {
     for (auto PredIt = BB->pred_begin(); PredIt != BB->pred_end(); ++PredIt)
@@ -185,8 +174,7 @@ template <> class SSAUpdaterTraits<DebugSSAUpdater> {
     return DbgValueDef();
   }
 
-  /// CreateEmptyPHI - Create a new PHI instruction in the specified block.
-  /// Reserve space for the operands (?) but do not fill them in yet.
+  /// CreateEmptyPHI - Create a new debug PHI entry for the specified block.
   static DbgSSAPhi *CreateEmptyPHI(DbgSSABlock *BB, unsigned NumPreds,
                                    DebugSSAUpdater *Updater) {
     DbgSSAPhi *PHI = BB->newPHI();
@@ -225,11 +213,10 @@ template <> class SSAUpdaterTraits<DebugSSAUpdater> {
 /// return it. If not, construct SSA form by first calculating the required
 /// placement of PHIs and then inserting new PHIs where needed.
 DbgValueDef DebugSSAUpdater::getValueAtEndOfBlockInternal(DbgSSABlock *BB) {
-  AvailableValsTy &AvailableVals = getAvailableVals(AV);
-  if (AvailableVals.contains(BB))
-    return AvailableVals[BB];
+  if (AV.contains(BB))
+    return AV[BB];
 
-  SSAUpdaterImpl<DebugSSAUpdater> Impl(this, &AvailableVals, InsertedPHIs);
+  SSAUpdaterImpl<DebugSSAUpdater> Impl(this, &AV, InsertedPHIs);
   return Impl.GetValue(BB);
 }
 
@@ -263,21 +250,13 @@ void DbgValueRangeTable::addVariable(Function *F, DebugVariableAggregate DVA) {
       for (DbgVariableRecord &DVR : filterDbgVars(I.getDbgRecordRange())) {
         if (DVR.getVariable() == Var &&
             DVR.getDebugLoc().getInlinedAt() == InlinedAt) {
-          assert(!DVR.isDbgAssign() && "No support for #dbg_declare yet.");
+          assert(!DVR.isDbgAssign() && "No support for #dbg_assign yet.");
           if (DVR.isDbgDeclare())
             DeclareRecordFound = true;
           ++NumRecordsFound;
           LastRecordFound = &DVR;
           DbgRecordValues.push_back(&DVR);
         }
-        if (!FoundInstructionInScope && I.getDebugLoc()) {
-          if (I.getDebugLoc().getInlinedAt() == InlinedAt &&
-              isContained(cast<DILocalScope>(I.getDebugLoc().getScope()),
-                          Var->getScope())) {
-            FoundInstructionInScope = true;
-            HasAnyInstructionsInScope.insert(&BB);
-          }
-        }
       }
       if (!FoundInstructionInScope && I.getDebugLoc()) {
         if (I.getDebugLoc().getInlinedAt() == InlinedAt &&
@@ -312,7 +291,8 @@ void DbgValueRangeTable::addVariable(Function *F, DebugVariableAggregate DVA) {
     return;
   }
 
-  // We don't have a single location, so let's have fun with liveness.
+  // We don't have a single location for the variable's entire scope, so instead
+  // we must now perform a liveness analysis to create a location list.
   DenseMap<BasicBlock *, DbgValueDef> LiveInMap;
   SmallVector<DbgSSAPhi *> HypotheticalPHIs;
   DebugSSAUpdater SSAUpdater(&HypotheticalPHIs);
@@ -395,21 +375,17 @@ void DbgValueRangeTable::printValues(DebugVariableAggregate DVA,
   }
 }
 
-uint64_t DbgValueRangeTable::addVariableName(Value *V, uint64_t Size) {
-  uint64_t Key = 0;
-  auto I = VariableMapping.find(V);
-  if (I == VariableMapping.end()) {
-    Key = KeyIndex;
-    VariableMapping.try_emplace(V, Key);
-    std::string &ValueText = VariableNameMapping[Key];
-    raw_string_ostream Stream(ValueText);
-    Stream << " ";
-    V->printAsOperand(Stream, true);
-    KeyIndex += Size;
-  } else {
-    Key = I->second;
-  }
-  LLVM_DEBUG(dbgs() << "Stashing Value: " << Key << " - " << *V << "\n");
-
-  return Key;
+SSAValueNameMap::ValueID SSAValueNameMap::addValue(Value *V) {
+  auto ExistingID = ValueToIDMap.find(V);
+  if (ExistingID != ValueToIDMap.end())
+    return ExistingID->second;
+  // First, get a new ID and Map V to it.
+  ValueID NewID = NextID++;
+  ValueToIDMap.insert({V, NewID});
+  // Then, get the name string for V and map NewID to it.
+  assert(!ValueIDToNameMap.contains(NewID) && "New value ID already maps to a name?");
+  std::string &ValueText = ValueIDToNameMap[NewID];
+  raw_string_ostream Stream(ValueText);
+  V->printAsOperand(Stream, true);
+  return NewID;
 }

llvm/unittests/Transforms/Utils/DebugSSAUpdaterTest.cpp

@@ -15,7 +15,6 @@
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/Module.h"
-#include "llvm/Support/Debug.h"
 #include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"