[RemoveDIs] Update DIBuilder to conditionally insert DbgRecords #84739

OCHyams · 2024-03-11T10:17:08Z

Have DIBuilder conditionally insert either debug intrinsics or DbgRecord depending on the module's IsNewDbgInfoFormat flag. The insertion methods now return a DbgInstPtr (a PointerUnion<Instruction *, DbgRecord *>).

Add a unittest for both modes (I couldn't find an existing test testing insertion behaviours specifically).

Update the DIBuilder C API. I went a different direction for this, rather than returning a union, I've added an additional set of functions.

LLVMDIBuilderInsertDeclareBefore            # Changed - Inserts a non-instruction debug record.
LLVMDIBuilderInsertDeclareAtEnd             # Changed - Inserts a non-instruction debug record.
LLVMDIBuilderInsertDbgValueBefore           # Changed - Inserts a non-instruction debug record.
LLVMDIBuilderInsertDbgValueAtEnd            # Changed - Inserts a non-instruction debug record.

LLVMIsNewDbgInfoFormat                      # New - Returns true if the module is in the new non-instruction mode.  Will be deprecated in future.
LLVMDIBuilderInsertDeclareIntrinsicBefore   # New - Old behaviour of the changed functions above, i.e., insert a dbg intrinsic call. Will be deprecated in future.
LLVMDIBuilderInsertDeclareIntrinsicAtEnd    # New - Old behaviour of the changed functions above, i.e., insert a dbg intrinsic call. Will be deprecated in future.
LLVMDIBuilderInsertDbgValueIntrinsicBefore  # New - Old behaviour of the changed functions above, i.e., insert a dbg intrinsic call. Will be deprecated in future.
LLVMDIBuilderInsertDbgValueIntrinsicAtEnd   # New - Old behaviour of the changed functions above, i.e., insert a dbg intrinsic call. Will be deprecated in future.

I figured this would minimise disruption to the C API, because we'll be removing debug intrinsics entirely at some point. However, if anyone strongly feels the approach should match the DIBuilder returning-a-union interface I can change it, I'm not particularly attached to either method.

This patch changes the existing assumption that DbgRecords are only ever inserted if there's an instruction to insert-before because clang currently inserts debug intrinsics while CodeGening (like any other instruction) meaning it'll try inserting to the end of a block without a terminator. We already have machinery in place to maintain the DbgRecords when a terminator is removed - these become "trailing DbgRecords" which are re-attached when a new instruction is inserted. All I've done is allow this state to occur while inserting DbgRecords too, i.e., it's not only removing terminators that causes this valid transient state, but inserting DbgRecords into incomplete blocks too.

Note: this doesn't mean clang is emitting DbgRecords yet, because the modules it creates are still always in the old debug mode. That will come in a future patch.

…also only want to do this if no terminator is present and the inst is at the end

This reverts commit 8b8cddb.

…el_codegen.cpp

SLTozer

Is it possible that the C-interface changes could be split into a separate patch? My concern is that it's going to be a very disruptive patch, and so the commit that breaks everyone using the llvm-c interface should be as small and specific as possible.

OCHyams · 2024-03-11T14:47:35Z

Is it possible that the C-interface changes could be split into a separate patch? My concern is that it's going to be a very disruptive patch, and so the commit that breaks everyone using the llvm-c interface should be as small and specific as possible.

SGTM. We could even consider leaving the C interface in an "old-format-only-mode" until we completely remove the "old format". The only thing that'd need to change in the C API at that point would be the return type of those functions. Looking at usages of DIBuilder in LLVM and Clang, the return type isn't used very often, so applying that update to a codebase using the builder API in a similar fashion would be trivial (there may be no work to do in a lot of cases). wdyt?

OCHyams · 2024-03-11T15:19:40Z

Rebased, removed C-API changes as mentioned above.

SLTozer · 2024-03-11T16:53:28Z

SGTM. We could even consider leaving the C interface in an "old-format-only-mode" until we completely remove the "old format".

That might work - though since the work is already done, perhaps the right order would be to have a patch that adds the RemoveDI-version of the C interface and an intrinsic-focused version, then in another patch change the original function to be the RemoveDIs version - that way the downstream consumers get a nudge, but have an immediate fix if needed (change instances of the existing function to the intrinsic version), then eventually we can delete the extra versions. Does that sound reasonable?

SLTozer

Largely looks good, some comments inline.

SLTozer · 2024-03-11T16:54:28Z

llvm/lib/IR/BasicBlock.cpp

-    assert(Dest != end() &&
-           "Transferring trailing DPValues to another trailing position");


Is this something that we see happen when building BBs in clang?

This is hit by test OpenMP/cancel_codegen.cpp

SLTozer · 2024-03-11T16:56:01Z

llvm/lib/IR/BasicBlock.cpp

-  assert(Where->getParent() == this);
-  if (!Where->DbgMarker)
-    createMarker(Where);
+  assert(Where == end() || Where->getParent() == this);


Is the first condition in this assert meant to be inverted? I would have thought that if Where == end(), then Where->getParent() == this would be trivially true!

If Where == end() then we can't dereference Where to check the parent matches. It's checking either Where is end() or, if it's not then the parent should match this bb.

Ah, so this is changing the condition here to account for insertions at end() (as for the OpenMP test above), while also preventing the error you just described. Got it!

SLTozer · 2024-03-11T17:07:15Z

llvm/lib/IR/DIBuilder.cpp

+      InsertBB->insertDPValueBefore(DPL, InsertBefore->getIterator());
+    else if (InsertBB)
+      InsertBB->insertDPValueBefore(DPL, InsertBB->end());
+    // FIXME: Use smart pointers for DbgRecord ownership management.


What would we need smart pointers for here?

Ah that was a leftover comment to myself. Not needed here, was just musing on whether introducing smart pointers to type-ify DbgRecod ownership might be beneficial in the future. Deleted the comment!

SLTozer · 2024-03-11T17:30:44Z

llvm/lib/Transforms/Scalar/SROA.cpp

+                            Dest,
+                            DIExpression::get(Expr->getContext(), std::nullopt),
+                            DbgAssign->getDebugLoc())
+            .template get<decltype(DbgInstType)>());


C++20 can't come soon enough. That being said, instead of passing the extra DbgInstType to this class, you could create an overloaded function just above this function body:

DPValue *UnwrapDbgInstPtr(DbgInstPtr P, DPValue *) { return static_cast<DPValue*>(cast<DbgRecord*>(P)); } `DPValue *UnwrapDbgInstPtr(DbgInstPtr P, DbgAssignIntrinsic *) { return static_cast<DbgAssignIntrinsic*>(cast<Instruction*>(P)); }

And then you could just call UnwrapDbgInstPtr(DIB.insertDbgAssign(...), DbgAssign) here. YMMV on this though!

SGTM, that moves some of the ugliness out of this lambda, which is desirable. Done.

SLTozer · 2024-03-11T17:37:21Z

llvm/unittests/IR/IRBuilderTest.cpp

+
+  // Test in new-debug mode.
+  // Reset the test then call convertToNewDbgValues to flip the flag
+  // on the test's Module, Funciton and BasicBlock.


Suggested change

// on the test's Module, Funciton and BasicBlock.

// on the test's Module, Function and BasicBlock.

SLTozer · 2024-03-11T17:38:04Z

llvm/unittests/IR/IRBuilderTest.cpp

+  // on the test's Module, Funciton and BasicBlock.
+  TearDown();
+  SetUp();
+  M->convertToNewDbgValues();


Do we need to convert M back afterwards? In some of the unit tests we do this (restore the previous state), but I'm not sure if these ones require it.

Good question but no not needed here. The module is deleted and recreated automatically for each test with TearDown and SetUp.

OCHyams

Does that sound reasonable?

SGTM

OCHyams · 2024-03-12T09:06:09Z

llvm/lib/IR/BasicBlock.cpp

-    assert(Dest != end() &&
-           "Transferring trailing DPValues to another trailing position");


This is hit by test OpenMP/cancel_codegen.cpp

OCHyams · 2024-03-12T09:12:18Z

llvm/lib/IR/DIBuilder.cpp

+      InsertBB->insertDPValueBefore(DPL, InsertBefore->getIterator());
+    else if (InsertBB)
+      InsertBB->insertDPValueBefore(DPL, InsertBB->end());
+    // FIXME: Use smart pointers for DbgRecord ownership management.


Ah that was a leftover comment to myself. Not needed here, was just musing on whether introducing smart pointers to type-ify DbgRecod ownership might be beneficial in the future. Deleted the comment!

OCHyams · 2024-03-12T09:16:35Z

llvm/unittests/IR/IRBuilderTest.cpp

+  // on the test's Module, Funciton and BasicBlock.
+  TearDown();
+  SetUp();
+  M->convertToNewDbgValues();


Good question but no not needed here. The module is deleted and recreated automatically for each test with TearDown and SetUp.

OCHyams · 2024-03-12T09:39:32Z

llvm/lib/Transforms/Scalar/SROA.cpp

+                            Dest,
+                            DIExpression::get(Expr->getContext(), std::nullopt),
+                            DbgAssign->getDebugLoc())
+            .template get<decltype(DbgInstType)>());


SGTM, that moves some of the ugliness out of this lambda, which is desirable. Done.

OCHyams · 2024-03-12T09:41:54Z

llvm/lib/IR/BasicBlock.cpp

-  assert(Where->getParent() == this);
-  if (!Where->DbgMarker)
-    createMarker(Where);
+  assert(Where == end() || Where->getParent() == this);


If Where == end() then we can't dereference Where to check the parent matches. It's checking either Where is end() or, if it's not then the parent should match this bb.

OCHyams · 2024-03-12T10:24:04Z

Thanks for the review!

This patch fixes problems that pop up when clang emits DbgRecords instead of debug intrinsics. Note: this doesn't mean clang is emitting DbgRecords yet, because the modules it creates are still always in the old debug mode. That will come in a future patch. Depends on #84739

…4915) Follow on from #84739, which updates the DIBuilder class. All the functions that have been added are temporary and will be deprecated in the future. The intention is that they'll help downstream projects adapt during the transition period. ``` New functions (all to be deprecated) ------------------------------------ LLVMIsNewDbgInfoFormat # Returns true if the module is in the new non-instruction mode. LLVMSetIsNewDbgInfoFormat # Convert to the requested debug info format. LLVMDIBuilderInsertDeclareIntrinsicBefore # Insert a debug intrinsic (old debug info format). LLVMDIBuilderInsertDeclareIntrinsicAtEnd # Same as above. LLVMDIBuilderInsertDbgValueIntrinsicBefore # Same as above. LLVMDIBuilderInsertDbgValueIntrinsicAtEnd # Same as above. LLVMDIBuilderInsertDeclareRecordBefore # Insert a debug record (new debug info format). LLVMDIBuilderInsertDeclareRecordAtEnd # Same as above. LLVMDIBuilderInsertDbgValueRecordBefore # Same as above. LLVMDIBuilderInsertDbgValueRecordAtEnd # Same as above. ``` The existing `LLVMDIBuilderInsert...` functions call through to the intrinsic versions (old debug info format) currently. In the next patch, I'll swap them to call the debug records versions (new debug info format). Downstream users of this API can query and change the current format using the first two functions above, or can instead opt to temporarily use intrinsics or records explicitly.

OCHyams added 30 commits March 10, 2024 21:28

Add DbgInstPtr union, update labels

b25c546

Add insertLabel body

b503e84

labels test try#1

3964cc5

continue to work on labeles test

36c7aa0

flushTerminators after every new instruction is inserted -- possibly …

a706ca6

…also only want to do this if no terminator is present and the inst is at the end

do what that commit says

c3cec69

Allow insert-before at end() and test it in unittest -- needed for clang

f443a18

tidy up the label test

88f6e21

dbg.values WIP: mem2reg and C API need attendance

c6bbfe4

add unittest for dbg.vals

035630f

declare API. Sans C-API. Needs test and impl

1f95a01

add insertDPValue helper

eb428e6

declare impl + tidy some others

42f4e74

dbg.declare unittest

063f9cd

dbg.assign api and impl

60f9a2f

assign test -not working

3a15cfa

fix dbg.assign test

7754eb9

insert dpassigns 'after' in DIBuilder

0a96251

gross C++

44008e2

C-API approach #1 - return a union

2e69a99

Revert "C-API approach #1 - return a union"

8b03c20

This reverts commit 8b8cddb.

other C-API tactic: use 2 function falvours

63511e0

add C API info to the docs

197ab57

allow empty to empty splice with trailing dpvalues, fixes OpenMP/canc…

ef7ce1a

…el_codegen.cpp

Fix clang/test/Frontend/stack-layout-remark.c

333bc9e

format

5cc4a3e

tidy

4e0bfff

fix comment

905a794

fix leftover dev comment/code

a7bbba3

undo uneeded change

edecd14

OCHyams added 6 commits March 10, 2024 21:34

Remove C-API changes

25c66b1

undo over-excited clang-format

04a4e29

remove sneaky leftover

e12c7dc

fix comments and assertions

bf1c8c1

SROA suggestion

8ea9e72

undo assertion change

ba210ae

OCHyams requested review from SLTozer, jmorse, jryans and nikic March 11, 2024 10:17

OCHyams mentioned this pull request Mar 11, 2024

[RemoveDIs] Update Clang front end to handle DbgRecords #84756

Merged

SLTozer reviewed Mar 11, 2024

View reviewed changes

OCHyams force-pushed the ddd-di-builder-upstream branch from 9adb5b7 to 25c66b1 Compare March 11, 2024 15:19

SLTozer reviewed Mar 11, 2024

View reviewed changes

OCHyams commented Mar 12, 2024

View reviewed changes

SLTozer approved these changes Mar 12, 2024

View reviewed changes

OCHyams merged commit 9997e03 into llvm:main Mar 12, 2024

OCHyams mentioned this pull request Mar 12, 2024

[RemoveDIs] Update DIBuilder C API with DbgRecord functions [1/2] #84915

Merged

		assert(Dest != end() &&
		"Transferring trailing DPValues to another trailing position");

	// on the test's Module, Funciton and BasicBlock.
	// on the test's Module, Function and BasicBlock.

[RemoveDIs] Update DIBuilder to conditionally insert DbgRecords #84739

[RemoveDIs] Update DIBuilder to conditionally insert DbgRecords #84739

Uh oh!

Conversation

OCHyams commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SLTozer left a comment

Choose a reason for hiding this comment

Uh oh!

OCHyams commented Mar 11, 2024

Uh oh!

OCHyams commented Mar 11, 2024

Uh oh!

SLTozer commented Mar 11, 2024

Uh oh!

SLTozer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OCHyams commented Mar 12, 2024

Uh oh!

Uh oh!

OCHyams commented Mar 11, 2024 •

edited

Loading