Skip to content

Conversation

@jmorse
Copy link
Member

@jmorse jmorse commented Nov 15, 2023

This trivial patch covers a bit of fallout from https://reviews.llvm.org/D153990, where we moved the storage of trailing DPValues into LLVMContext rather than being stored in each block. As a result, you can now get a null DPMarker pointer from the end() iterator where previously you didn't (and now it has to be explicitly created).

This is a sort-of stopgap measure -- there's another all-singing all-dancing patch further down the line that refactors all of this so that we don't allocate a DPMarker for every single Instruction. When that lands this will all be refactored so that every time we request a DPMarker, one is created if needs be. That's a performance-fix rather than a functionality related patch though, so it'll come later.

This patch covers a bit of fallout from https://reviews.llvm.org/D153990,
where we moved the storage of trailing DPValues into LLVMContext rather
than being stored in each block. As a result, you can now get a null
DPMarker pointer from the end() iterator where previously you didn't (and
now it has to be explicitly created).

This is a sort-of stopgap measure -- there's another patch further down the
line that refactors all of this so that we don't allocate a DPMarker for
every single Instruction. When that lands this will all be refactored so
that every time we request a DPMarker, one is created if needs be. That's a
performance-fix rather than a functionality related patch though, so it'll
come later.
@jmorse jmorse requested review from OCHyams and SLTozer November 15, 2023 13:21
@llvmbot
Copy link
Member

llvmbot commented Nov 15, 2023

@llvm/pr-subscribers-llvm-ir

Author: Jeremy Morse (jmorse)

Changes

This trivial patch covers a bit of fallout from https://reviews.llvm.org/D153990, where we moved the storage of trailing DPValues into LLVMContext rather than being stored in each block. As a result, you can now get a null DPMarker pointer from the end() iterator where previously you didn't (and now it has to be explicitly created).

This is a sort-of stopgap measure -- there's another all-singing all-dancing patch further down the line that refactors all of this so that we don't allocate a DPMarker for every single Instruction. When that lands this will all be refactored so that every time we request a DPMarker, one is created if needs be. That's a performance-fix rather than a functionality related patch though, so it'll come later.


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

1 Files Affected:

  • (modified) llvm/lib/IR/Instruction.cpp (+2)
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 27b2c5ee4d399dc..7449692f05d7bf9 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -146,6 +146,8 @@ void Instruction::insertBefore(BasicBlock &BB,
   bool InsertAtHead = InsertPos.getHeadBit();
   if (!InsertAtHead) {
     DPMarker *SrcMarker = BB.getMarker(InsertPos);
+    if (!SrcMarker)
+      SrcMarker = BB.createMarker(InsertPos);
     DbgMarker->absorbDebugValues(*SrcMarker, false);
   }
 

Copy link
Contributor

@OCHyams OCHyams left a comment

Choose a reason for hiding this comment

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

LGTM

@jmorse jmorse merged commit c2a2fd2 into llvm:main Nov 15, 2023
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
)

This trivial patch covers a bit of fallout from
https://reviews.llvm.org/D153990, where we moved the storage of trailing
DPValues into LLVMContext rather than being stored in each block. As a
result, you can now get a null DPMarker pointer from the end() iterator
where previously you didn't (and now it has to be explicitly created).

This is a sort-of stopgap measure -- there's another all-singing
all-dancing patch further down the line that refactors all of this so
that we don't allocate a DPMarker for every single Instruction. When
that lands this will all be refactored so that every time we request a
DPMarker, one is created if needs be. That's a performance-fix rather
than a functionality related patch though, so it'll come later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants