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

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-debuginfo
            
<details>
<summary>Changes</summary>
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.
--
Full diff: https://github.com/llvm/llvm-project/pull/66433.diff

2 Files Affected:

- (modified) llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (+16-30) 
- (modified) llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h (+6-11) 


<pre>
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 1cb65a8a9a659f3..1039c5955e905d7 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -257,6 +257,19 @@ static DbgValueLoc getDebugLocValue(const MachineInstr *MI) {
   return DbgValueLoc(Expr, DbgValueLocEntries, IsVariadic);
 }
 
+static uint64_t getFragmentOffsetInBits(const DIExpression &amp;Expr) {
+  std::optional&lt;DIExpression::FragmentInfo&gt; Fragment = Expr.getFragmentInfo();
+  return Fragment ? Fragment-&gt;OffsetInBits : 0;
+}
+
+bool FrameIndexExpr::operator&lt;(const FrameIndexExpr &amp;Other) const {
+  return getFragmentOffsetInBits(*Expr) &lt; getFragmentOffsetInBits(*Other.Expr);
+}
+
+bool EntryValueInfo::operator&lt;(const EntryValueInfo &amp;Other) const {
+  return getFragmentOffsetInBits(Expr) &lt; getFragmentOffsetInBits(Other.Expr);
+}
+
 Loc::Single::Single(DbgValueLoc ValueLoc)
     : ValueLoc(std::make_unique&lt;DbgValueLoc&gt;(ValueLoc)),
       Expr(ValueLoc.getExpression()) {
@@ -267,42 +280,15 @@ Loc::Single::Single(DbgValueLoc ValueLoc)
 Loc::Single::Single(const MachineInstr *DbgValue)
     : Single(getDebugLocValue(DbgValue)) {}
 
-ArrayRef&lt;FrameIndexExpr&gt; Loc::MMI::getFrameIndexExprs() const {
-  if (FrameIndexExprs.size() == 1)
-    return FrameIndexExprs;
-
-  assert(llvm::all_of(
-             FrameIndexExprs,
-             [](const FrameIndexExpr &amp;A) { return A.Expr-&gt;isFragment(); }) &amp;&amp;
-         &quot;multiple FI expressions without DW_OP_LLVM_fragment&quot;);
-  llvm::sort(FrameIndexExprs,
-             [](const FrameIndexExpr &amp;A, const FrameIndexExpr &amp;B) -&gt; bool {
-               return A.Expr-&gt;getFragmentInfo()-&gt;OffsetInBits &lt;
-                      B.Expr-&gt;getFragmentInfo()-&gt;OffsetInBits;
-             });
-
+const std::set&lt;FrameIndexExpr&gt; &amp;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-&gt;isFragment())
-      return;
-  }
-
-  if (llvm::none_of(FrameIndexExprs, [&amp;](const FrameIndexExpr &amp;Other) {
-        return FI == Other.FI &amp;&amp; Expr == Other.Expr;
-      }))
-    FrameIndexExprs.push_back({FI, Expr});
-
+  FrameIndexExprs.insert({FI, Expr});
   assert((FrameIndexExprs.size() == 1 ||
           llvm::all_of(FrameIndexExprs,
-                       [](FrameIndexExpr &amp;FIE) {
+                       [](const FrameIndexExpr &amp;FIE) {
                          return FIE.Expr &amp;&amp; FIE.Expr-&gt;isFragment();
                        })) &amp;&amp;
          &quot;conflicting locations for variable&quot;);
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
index e8b0f3178939dc8..a60f0816798f336 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.
+  bool operator&lt;(const FrameIndexExpr &amp;Other) const;
 };
 
 /// Represents an entry-value location, or a fragment of one.
@@ -115,15 +118,7 @@ struct EntryValueInfo {
   const DIExpression &amp;Expr;
 
   /// Operator enabling sorting based on fragment offset.
-  bool operator&lt;(const EntryValueInfo &amp;Other) const {
-    return getFragmentOffsetInBits() &lt; Other.getFragmentOffsetInBits();
-  }
-
-private:
-  uint64_t getFragmentOffsetInBits() const {
-    std::optional&lt;DIExpression::FragmentInfo&gt; Fragment = Expr.getFragmentInfo();
-    return Fragment ? Fragment-&gt;OffsetInBits : 0;
-  }
+  bool operator&lt;(const EntryValueInfo &amp;Other) const;
 };
 
 // Namespace for alternatives of a DbgVariable.
@@ -158,7 +153,7 @@ class Multi {
 };
 /// Single location defined by (potentially multiple) MMI entries.
 struct MMI {
-  mutable SmallVector&lt;FrameIndexExpr, 1&gt; FrameIndexExprs;
+  std::set&lt;FrameIndexExpr&gt; 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&lt;FrameIndexExpr&gt; getFrameIndexExprs() const;
+  const std::set&lt;FrameIndexExpr&gt; &amp;getFrameIndexExprs() const;
 };
 /// Single location defined by (potentially multiple) EntryValueInfo.
 struct EntryValue {
</pre>
</details>


https://github.com/llvm/llvm-project/pull/66433