[llvm] 434907e - [NFC][AsmPrinter] Refactor FrameIndexExprs as a std::set (#66433)

[via llvm-commits]

Author: Scott Linder
Date: 2023-09-20T12:05:05-04:00
New Revision: 434907e474a823ee4d96ca0aa4f30992fb03a3b8

URL: https://github.com/llvm/llvm-project/commit/434907e474a823ee4d96ca0aa4f30992fb03a3b8
DIFF: https://github.com/llvm/llvm-project/commit/434907e474a823ee4d96ca0aa4f30992fb03a3b8.diff

LOG: [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.

Added: 
    

Modified: 
    llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
    llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

Removed: 
    


################################################################################
diff  --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 05fb219c7a47b6b..ee2ab71ad28e47f 100644
--- 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 
diff erent 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
index e8b0f3178939dc8..52b342fe150fa57 100644
--- 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 {