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

[NFC] Move DIExpressionCursor to DebugInfoMetadata.h #69768

Merged
merged 1 commit into from
May 29, 2024

Conversation

rastogishubham
Copy link
Contributor

@rastogishubham rastogishubham commented Oct 20, 2023

This is an NFC patch to move DIExpressionCursor to DebugInfoMetada.h, so that it can be used by classes in that header file.

Specifically, I want to use DIExpressionCursor in a subsequent patch:
#71718

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 20, 2023

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Shubham Sandeep Rastogi (rastogishubham)

Changes

This is an NFC patch to move DIExpressionCursor to DebugInfoMetada.h, so that it can be used by classes in that header file.

Specifically, I want to use DIExpressionCursor in a subsequent patche:
#69769


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

2 Files Affected:

  • (modified) llvm/include/llvm/IR/DebugInfoMetadata.h (+61)
  • (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..86ee9f004cfd42b 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -3073,6 +3073,67 @@ 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;
+  }
+
+  /// 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
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

Simple enough (maybe wait on committing this until the usage is approved - or roll it back if the usage doesn't eventuate (if subsequent patches go in a different direction, etc))

@MaskRay
Copy link
Member

MaskRay commented Feb 28, 2024

@rastogishubham This has been approved long ago. Are you planning to re-run the tests (since old results are stale) and land it?

@rastogishubham
Copy link
Contributor Author

@MaskRay I have subsequent patches that build on top of this change that I had to stop working on because of some other work so I didn't merge this patch as well, do you suggest I merge this patch or wait?

@rastogishubham rastogishubham merged commit 8912920 into llvm:main May 29, 2024
4 checks passed
@rastogishubham rastogishubham deleted the MoveCursor branch May 29, 2024 22:36
rastogishubham added a commit that referenced this pull request May 29, 2024
…>) to DIExpressionCursor (#71717)

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

5 participants