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

slinder1 · 2023-09-14T21:09:50Z

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.

llvmbot · 2023-09-14T21:10:58Z

@llvm/pr-subscribers-debuginfo

Changes

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)

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 {

SLTozer

Definitely looks nicer with std::set than the mutable; if compile times are a concern, you can also try using the LLVM compile time tracker - you could get set up to use it to test commits pre-merge, but you can also just see if your patch causes any large changes post-merge (which is fine imo given the numbers you posted on the earlier patch).

dwblaikie · 2023-09-16T05:07:34Z

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<(const FrameIndexExpr &Other) const;


Usually it's preferable to implement any operator overload that can be a non-member, as a non-member (to allow equal conversions on both the LHS and RHS) - could you do that here? (& elsewhere)

Sounds good! I rolled the change to the other type here into this PR as it seems trivial and the PR process in GH doesn't let me "stack" it easily. Let me know if anyone prefers it go in separately!

dwblaikie · 2023-09-16T05:08:20Z

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

@@ -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;


Another possibilty would be a SetVector? (though then it's insertion ordered, rather than < ordered - not sure if that's sufficiently stable for the needs? perhaps not)

I think we don't need the extra functionality provided by the SetVector here: our sorting keys (fragment offsets) are stable without the need for insertion-order-tracking.

IIUC the DWARF emitter also wants the fragments ordered by offset? I'm not sure what would happen if we were to emit DWARF fragment expressions out of order 🤔

IIUC the DWARF emitter also wants the fragments ordered by offset? I'm not sure what would happen if we were to emit DWARF fragment expressions out of order 🤔

Ah, yeah - DWARF does require the fragments be in order (essentially they don't say where they are in the structure - so you have to emit the first byte first, second byte second, etc)

felipepiovezan

LGTM! Glad to see this change.

(also, sorry for the delay... I still haven't sorted my github email filters)

slinder1 · 2023-09-18T20:55:26Z

Definitely looks nicer with std::set than the mutable; if compile times are a concern, you can also try using the LLVM compile time tracker - you could get set up to use it to test commits pre-merge, but you can also just see if your patch causes any large changes post-merge (which is fine imo given the numbers you posted on the earlier patch).

Thank you for the pointer! Nikita graciously added my fork and I tried it out on this branch. The worst benchmark for instructions:u is +0.03% and it all seems well within the noise.

slinder1 · 2023-09-19T20:57:19Z

It seems like I ran into the issue mentioned in https://discourse.llvm.org/t/how-to-rerun-buildkite-github-pull-requests/73402/4 with the windows build-bot

I'm going to force-push to the branch to kick off a new build

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.

slinder1 requested a review from felipepiovezan September 14, 2023 21:09

llvmbot added the debuginfo label Sep 14, 2023

slinder1 requested a review from adrian-prantl September 14, 2023 21:13

SLTozer approved these changes Sep 15, 2023

View reviewed changes

dwblaikie reviewed Sep 16, 2023

View reviewed changes

felipepiovezan approved these changes Sep 16, 2023

View reviewed changes

dwblaikie approved these changes Sep 18, 2023

View reviewed changes

slinder1 force-pushed the dbgvariable-variant-refactor_vec-to-set branch from a3f4517 to 7e65050 Compare September 19, 2023 20:58

slinder1 merged commit 434907e into llvm:main Sep 20, 2023

slinder1 deleted the dbgvariable-variant-refactor_vec-to-set branch September 20, 2023 16:05

This was referenced Sep 20, 2023

sve gather scatter offset sinking #66932

Closed

sve gather scatter offset sinking #66994

Closed

kstoimenov mentioned this pull request Sep 22, 2023

Add memcpm test kstoimenov/llvm-project#13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

slinder1 commented Sep 14, 2023

llvmbot commented Sep 14, 2023

An old FIXME and accompanying workaround are dropped. It is ostensibly dead-code within the current codebase.

SLTozer left a comment

dwblaikie Sep 16, 2023

slinder1 Sep 18, 2023

dwblaikie Sep 16, 2023

felipepiovezan Sep 16, 2023

felipepiovezan Sep 16, 2023

dwblaikie Sep 18, 2023

felipepiovezan left a comment •

edited

Loading

slinder1 commented Sep 18, 2023

slinder1 commented Sep 19, 2023

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

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

Conversation

slinder1 commented Sep 14, 2023

llvmbot commented Sep 14, 2023

An old FIXME and accompanying workaround are dropped. It is ostensibly dead-code within the current codebase.

SLTozer left a comment

Choose a reason for hiding this comment

dwblaikie Sep 16, 2023

Choose a reason for hiding this comment

slinder1 Sep 18, 2023

Choose a reason for hiding this comment

dwblaikie Sep 16, 2023

Choose a reason for hiding this comment

felipepiovezan Sep 16, 2023

Choose a reason for hiding this comment

felipepiovezan Sep 16, 2023

Choose a reason for hiding this comment

dwblaikie Sep 18, 2023

Choose a reason for hiding this comment

felipepiovezan left a comment • edited Loading

Choose a reason for hiding this comment

slinder1 commented Sep 18, 2023

slinder1 commented Sep 19, 2023

felipepiovezan left a comment •

edited

Loading