Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functions peekNextN(unsigned) and assignNewExpr(ArrayRef<uint64_t>) to DIExpressionCursor #71717

Merged
merged 1 commit into from
May 29, 2024

Conversation

rastogishubham
Copy link
Contributor

This commit adds two functions to the DIExpressionCursor class.

peekNextN(unsigned) works like peekNext, but lets you peek the next Nth element

assignNewExpr(ArrayRef<uint64_t>) lets you assign a new expression to the same DIExpressionCursor object

This is part of a stack of patches, it comes after
#69768

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 8, 2023

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Shubham Sandeep Rastogi (rastogishubham)

Changes

This commit adds two functions to the DIExpressionCursor class.

peekNextN(unsigned) works like peekNext, but lets you peek the next Nth element

assignNewExpr(ArrayRef&lt;uint64_t&gt;) lets you assign a new expression to the same DIExpressionCursor object

This is part of a stack of patches, it comes after
#69768


Full diff: https://github.com/llvm/llvm-project/pull/71717.diff

2 Files Affected:

  • (modified) llvm/include/llvm/IR/DebugInfoMetadata.h (+82)
  • (modified) llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h (-61)
diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index b347664883fd9f2..5ea8d853e86c854 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -3044,6 +3044,8 @@ class DIExpression : public MDNode {
   /// expression and constant on failure.
   std::pair<DIExpression *, const ConstantInt *>
   constantFold(const ConstantInt *CI);
+
+  DIExpression *constantFold();
 };
 
 inline bool operator==(const DIExpression::FragmentInfo &A,
@@ -3073,6 +3075,86 @@ template <> struct DenseMapInfo<DIExpression::FragmentInfo> {
   static bool isEqual(const FragInfo &A, const FragInfo &B) { return A == B; }
 };
 
+/// Holds a DIExpression and keeps track of how many operands have been consumed
+/// so far.
+class DIExpressionCursor {
+  DIExpression::expr_op_iterator Start, End;
+
+public:
+  DIExpressionCursor(const DIExpression *Expr) {
+    if (!Expr) {
+      assert(Start == End);
+      return;
+    }
+    Start = Expr->expr_op_begin();
+    End = Expr->expr_op_end();
+  }
+
+  DIExpressionCursor(ArrayRef<uint64_t> Expr)
+      : Start(Expr.begin()), End(Expr.end()) {}
+
+  DIExpressionCursor(const DIExpressionCursor &) = default;
+
+  /// Consume one operation.
+  std::optional<DIExpression::ExprOperand> take() {
+    if (Start == End)
+      return std::nullopt;
+    return *(Start++);
+  }
+
+  /// Consume N operations.
+  void consume(unsigned N) { std::advance(Start, N); }
+
+  /// Return the current operation.
+  std::optional<DIExpression::ExprOperand> peek() const {
+    if (Start == End)
+      return std::nullopt;
+    return *(Start);
+  }
+
+  /// Return the next operation.
+  std::optional<DIExpression::ExprOperand> peekNext() const {
+    if (Start == End)
+      return std::nullopt;
+
+    auto Next = Start.getNext();
+    if (Next == End)
+      return std::nullopt;
+
+    return *Next;
+  }
+
+  std::optional<DIExpression::ExprOperand> peekNextN(unsigned N) const {
+    if (Start == End)
+      return std::nullopt;
+    DIExpression::expr_op_iterator Nth = Start;
+    for (unsigned I = 0; I < N; I++) {
+      Nth = Nth.getNext();
+      if (Nth == End)
+        return std::nullopt;
+    }
+    return *Nth;
+  }
+
+  void assignNewExpr(ArrayRef<uint64_t> Expr) {
+    DIExpression::expr_op_iterator NewStart(Expr.begin());
+    DIExpression::expr_op_iterator NewEnd(Expr.end());
+    this->Start = NewStart;
+    this->End = NewEnd;
+  }
+
+  /// Determine whether there are any operations left in this expression.
+  operator bool() const { return Start != End; }
+
+  DIExpression::expr_op_iterator begin() const { return Start; }
+  DIExpression::expr_op_iterator end() const { return End; }
+
+  /// Retrieve the fragment information, if any.
+  std::optional<DIExpression::FragmentInfo> getFragmentInfo() const {
+    return DIExpression::getFragmentInfo(Start, End);
+  }
+};
+
 /// Global variables.
 ///
 /// TODO: Remove DisplayName.  It's always equal to Name.
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
index 667a9efc6f6c04f..4daa78b15b8e298 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
@@ -31,67 +31,6 @@ class DIELoc;
 class TargetRegisterInfo;
 class MachineLocation;
 
-/// Holds a DIExpression and keeps track of how many operands have been consumed
-/// so far.
-class DIExpressionCursor {
-  DIExpression::expr_op_iterator Start, End;
-
-public:
-  DIExpressionCursor(const DIExpression *Expr) {
-    if (!Expr) {
-      assert(Start == End);
-      return;
-    }
-    Start = Expr->expr_op_begin();
-    End = Expr->expr_op_end();
-  }
-
-  DIExpressionCursor(ArrayRef<uint64_t> Expr)
-      : Start(Expr.begin()), End(Expr.end()) {}
-
-  DIExpressionCursor(const DIExpressionCursor &) = default;
-
-  /// Consume one operation.
-  std::optional<DIExpression::ExprOperand> take() {
-    if (Start == End)
-      return std::nullopt;
-    return *(Start++);
-  }
-
-  /// Consume N operations.
-  void consume(unsigned N) { std::advance(Start, N); }
-
-  /// Return the current operation.
-  std::optional<DIExpression::ExprOperand> peek() const {
-    if (Start == End)
-      return std::nullopt;
-    return *(Start);
-  }
-
-  /// Return the next operation.
-  std::optional<DIExpression::ExprOperand> peekNext() const {
-    if (Start == End)
-      return std::nullopt;
-
-    auto Next = Start.getNext();
-    if (Next == End)
-      return std::nullopt;
-
-    return *Next;
-  }
-
-  /// Determine whether there are any operations left in this expression.
-  operator bool() const { return Start != End; }
-
-  DIExpression::expr_op_iterator begin() const { return Start; }
-  DIExpression::expr_op_iterator end() const { return End; }
-
-  /// Retrieve the fragment information, if any.
-  std::optional<DIExpression::FragmentInfo> getFragmentInfo() const {
-    return DIExpression::getFragmentInfo(Start, End);
-  }
-};
-
 /// Base class containing the logic for constructing DWARF expressions
 /// independently of whether they are emitted into a DIE or into a .debug_loc
 /// entry.

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think we usually desire a test for each change, but I imagine there are tests coming later in the patch series. NB, I read the intermediate commit 05f6b6f rather than looking at the github "Files Changes" view, seeing how we can't have nice things like stacked reviews any more.

llvm/include/llvm/IR/DebugInfoMetadata.h Outdated Show resolved Hide resolved
@rastogishubham
Copy link
Contributor Author

@jmorse how does it look now?

Copy link
Collaborator

@pogo59 pogo59 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, @jmorse is on leave at this point.

return *Next;
}

std::optional<DIExpression::ExprOperand> peekNextN(unsigned N) const {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wants a doxygen comment. And maybe peekNextNth so as not to imply it returns the next N elements?

…>) to DIExpressionCursor

This commit adds two functions to the DIExpressionCursor class.

peekNextN(unsigned) works like peekNext, but lets you peek the next Nth
element

assignNewExpr(ArrayRef<uint64_t>) lets you assign a new expression to
the same DIExpressionCursor object
@rastogishubham rastogishubham merged commit a3f9066 into llvm:main May 29, 2024
4 of 6 checks passed
@rastogishubham rastogishubham deleted the NewFuncs branch May 29, 2024 22:42
rastogishubham added a commit that referenced this pull request May 29, 2024
DIExpressions can get very long and have a lot of redundant operations.
This function uses simple pattern matching to fold constant math that
can be evaluated at compile time.

The hope is that other people can contribute other patterns as well.

I also couldn't see a good way of combining this with
`DIExpression::constantFold` so it stands alone.

This is part of a stack of patches and comes after
#69768
#71717
rastogishubham added a commit that referenced this pull request May 29, 2024
…1719)

This patch uses `DIExpression::foldConstantMath()` at the end of a
`DIExpression::append()`. Which should help in reducing the size of
DIExpressions that grow because of salvaging debug info

This is part of a stack of patches and comes after:
#69768
#71717
#71718
alanzhao1 pushed a commit to alanzhao1/llvm-project that referenced this pull request May 29, 2024
…sion (llvm#71721)

This patch uses `DIExpression::foldConstantMath()` at the result of a
Salvaged expression, that is, it runs the folding optimizations after an
expression has been salvaged completely, to reduce how many times the
fold optimization function is called. Which should help in reducing the
size of DIExpressions that grow because of salvaging debug info

After checking the size of the dSYM with and without this change, I saw
a decrease of about 300KB, where the debug_loc section is about 1.6 GB
in size.

Where the debug loc section reduced in size by 212KB and it is 193MB in
size, the rest comes from the debug_info section

This is part of a stack of patches and comes after:
llvm#69768
llvm#71717
llvm#71718
llvm#71719
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants