[NFC][AsmPrinter] Refactor FrameIndexExprs as a std::set (#66433)

This avoids the need for a mutable member to implement deferred sorting, and some bespoke code to maintain a SmallVector as a set. The performance impact seems to be negligible in some small tests, and so seems acceptable to greatly simplify the code. An old FIXME and accompanying workaround are dropped. It is ostensibly dead-code within the current codebase.
llvm · Sep 20, 2023 · 434907e · 434907e
1 parent 309d1c4
commit 434907e
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 41 deletions.
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -265,6 +265,20 @@ static DbgValueLoc getDebugLocValue(const MachineInstr *MI) {
   return DbgValueLoc(Expr, DbgValueLocEntries, IsVariadic);
 }
 
+static uint64_t getFragmentOffsetInBits(const DIExpression &Expr) {
+  std::optional<DIExpression::FragmentInfo> Fragment = Expr.getFragmentInfo();
+  return Fragment ? Fragment->OffsetInBits : 0;
+}
+
+bool llvm::operator<(const FrameIndexExpr &LHS, const FrameIndexExpr &RHS) {
+  return getFragmentOffsetInBits(*LHS.Expr) <
+         getFragmentOffsetInBits(*RHS.Expr);
+}
+
+bool llvm::operator<(const EntryValueInfo &LHS, const EntryValueInfo &RHS) {
+  return getFragmentOffsetInBits(LHS.Expr) < getFragmentOffsetInBits(RHS.Expr);
+}
+
 Loc::Single::Single(DbgValueLoc ValueLoc)
     : ValueLoc(std::make_unique<DbgValueLoc>(ValueLoc)),
       Expr(ValueLoc.getExpression()) {
@@ -275,42 +289,15 @@ Loc::Single::Single(DbgValueLoc ValueLoc)
 Loc::Single::Single(const MachineInstr *DbgValue)
     : Single(getDebugLocValue(DbgValue)) {}
 
-ArrayRef<FrameIndexExpr> Loc::MMI::getFrameIndexExprs() const {
-  if (FrameIndexExprs.size() == 1)
-    return FrameIndexExprs;
-
-  assert(llvm::all_of(
-             FrameIndexExprs,
-             [](const FrameIndexExpr &A) { return A.Expr->isFragment(); }) &&
-         "multiple FI expressions without DW_OP_LLVM_fragment");
-  llvm::sort(FrameIndexExprs,
-             [](const FrameIndexExpr &A, const FrameIndexExpr &B) -> bool {
-               return A.Expr->getFragmentInfo()->OffsetInBits <
-                      B.Expr->getFragmentInfo()->OffsetInBits;
-             });
-
+const std::set<FrameIndexExpr> &Loc::MMI::getFrameIndexExprs() const {
   return FrameIndexExprs;
 }
 
 void Loc::MMI::addFrameIndexExpr(const DIExpression *Expr, int FI) {
-  // FIXME: This logic should not be necessary anymore, as we now have proper
-  // deduplication. However, without it, we currently run into the assertion
-  // below, which means that we are likely dealing with broken input, i.e. two
-  // non-fragment entries for the same variable at different frame indices.
-  if (FrameIndexExprs.size()) {
-    auto *Expr = FrameIndexExprs.back().Expr;
-    if (!Expr || !Expr->isFragment())
-      return;
-  }
-
-  if (llvm::none_of(FrameIndexExprs, [&](const FrameIndexExpr &Other) {
-        return FI == Other.FI && Expr == Other.Expr;
-      }))
-    FrameIndexExprs.push_back({FI, Expr});
-
+  FrameIndexExprs.insert({FI, Expr});
   assert((FrameIndexExprs.size() == 1 ||
           llvm::all_of(FrameIndexExprs,
-                       [](FrameIndexExpr &FIE) {
+                       [](const FrameIndexExpr &FIE) {
                          return FIE.Expr && FIE.Expr->isFragment();
                        })) &&
          "conflicting locations for variable");

diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
@@ -107,6 +107,9 @@ class DbgVariable;
 struct FrameIndexExpr {
   int FI;
   const DIExpression *Expr;
+
+  /// Operator enabling sorting based on fragment offset.
+  friend bool operator<(const FrameIndexExpr &LHS, const FrameIndexExpr &RHS);
 };
 
 /// Represents an entry-value location, or a fragment of one.
@@ -115,15 +118,7 @@ struct EntryValueInfo {
   const DIExpression &Expr;
 
   /// Operator enabling sorting based on fragment offset.
-  bool operator<(const EntryValueInfo &Other) const {
-    return getFragmentOffsetInBits() < Other.getFragmentOffsetInBits();
-  }
-
-private:
-  uint64_t getFragmentOffsetInBits() const {
-    std::optional<DIExpression::FragmentInfo> Fragment = Expr.getFragmentInfo();
-    return Fragment ? Fragment->OffsetInBits : 0;
-  }
+  friend bool operator<(const EntryValueInfo &LHS, const EntryValueInfo &RHS);
 };
 
 // Namespace for alternatives of a DbgVariable.
@@ -158,7 +153,7 @@ class Multi {
 };
 /// Single location defined by (potentially multiple) MMI entries.
 struct MMI {
-  mutable SmallVector<FrameIndexExpr, 1> FrameIndexExprs;
+  std::set<FrameIndexExpr> FrameIndexExprs;
 
 public:
   explicit MMI(const DIExpression *E, int FI) : FrameIndexExprs({{FI, E}}) {
@@ -167,7 +162,7 @@ struct MMI {
   }
   void addFrameIndexExpr(const DIExpression *Expr, int FI);
   /// Get the FI entries, sorted by fragment offset.
-  ArrayRef<FrameIndexExpr> getFrameIndexExprs() const;
+  const std::set<FrameIndexExpr> &getFrameIndexExprs() const;
 };
 /// Single location defined by (potentially multiple) EntryValueInfo.
 struct EntryValue {