This is an archive of the discontinued LLVM Phabricator instance.

[ADT][DebugInfo][RemoveDIs] Permit extra flags in ilist_iterator's for communicating debug-info facts
ClosedPublic

Authored by jmorse on Jun 26 2023, 8:14 AM.

Details

Summary

Hi,

This patch adds a new ilist_iterator -like class that can carry two extra bits as well as the usual node pointer. The rationale behind having these extra bits can be found here [0], the tl;dr being that they're needed to signal whether a "position" in a BasicBlock includes any debug-info before or after the iterator. It's fundamental to removing debug intrinsics, because we need to be able to signal extra information about "positions" in the block.

Major observations: this entirely duplicates ilist_iterator. I took a shot at re-use via inheritance, however there are so many places where the iterator type is re-constructed that most of the class has to be re-implemented anyway. I feel that de-duplicating these types would be a false economy, especially given that LLVM doesn't actively develop this iterator type on a regular basis.

I've made it enable-able through the existing ilist_node options interface, which means there are a fair few sites where the instruction-list type needs to be updated; I think this is just a one-off cost. By default everything will use ilist_iterator, except instruction lists that will use ilist_iterator_w_bits.

I've also made the "new bits" disable-able with a CMAKE flag, and off by default. The exact cost of these bits being present is something like 0.1% according to the LLVM compile-time-tracker (more than offset by future savings), but given that this overall (removing debug intrinsics) feature is going to be in development and evaluation for a while, it doesn't make sense for the compile-time cost to be paid up front.

~

On to the actual patch: as mentioned, this completely duplicates ilist_iterator, adding only the head-and-tail bits plus the accessors to them. There's a moderate amount of plumbing to join up the ilist_node options with selecting the correct iterator. Quite a lot of the change is normalising the different ways the instruction list can be named in different scenarios.

In terms of the functional changes: BasicBlock::begin and getFirstInsertionPt now will return iterators with the "head" bit set, signalling that the iterator was intended to include the debug-info at the start of the block. I've added a small unit-test to ensure that this information is stored, and preserved across copy-construction and assignment. Increment and decrement operations naturally construct a new iterator, dropping the head/tail bits in the process: this is the desired behaviour, as once the iterator is modified it's no longer intended to be at the "head" of the block.

For test coverage, the bit-setting facility is the only change in behaviour that exists, hence the small unit test. This only actually tests anything useful if the CMake flag is set to enable the presence of these bits. With this patch uploaded, I'm going to do some prodding to see whether we can get one of the sie buildbots running with this flag.

[0] https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939

Diff Detail

Event Timeline

jmorse created this revision.Jun 26 2023, 8:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 8:14 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jmorse requested review of this revision.Jun 26 2023, 8:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 8:14 AM
jmorse updated this revision to Diff 544025.Jul 25 2023, 10:13 AM

Add setHeadBit(true) to the getFirstNonPHIIt accessors I've added, to return the position of the first non-PHI at the start of the block.

Orlando accepted this revision.Sep 22 2023, 8:30 AM

LGTM (pending clang-formatting if not already, plus a few nits).

The exact cost of these bits being present is something like 0.1% according to the LLVM compile-time-tracker (more than offset by future savings), but given that this overall (removing debug intrinsics) feature is going to be in development and evaluation for a while, it doesn't make sense for the compile-time cost to be paid up front.

This makes sense to me.

llvm/CMakeLists.txt
627

nit: no ilist_iterator's -> ilist_iterators, but the singular instead works too imo.

llvm/include/llvm/IR/SymbolTableListTraits.h
114–118

has this been clang-formatted?

llvm/lib/IR/BasicBlock.cpp
225

Worth copying the comment you added to getFirstInsertionPt here too?

llvm/unittests/ADT/IListIteratorBitsTest.cpp
100–101

Comment floated here from elsewhere?

This revision is now accepted and ready to land.Sep 22 2023, 8:30 AM

(NB: I'm also going to try and get one of our buildbots building with the cmake flag set so that this doesn't immediately commit uncovered code.)

jmorse added a comment.Oct 4 2023, 3:40 AM

Said buildbot PR: https://github.com/llvm/llvm-zorg/pull/48

(I've still got the comment-fixes to make too)

dyung added a subscriber: dyung.Oct 7 2023, 12:24 PM
jmorse marked an inline comment as done.Oct 17 2023, 7:04 AM

@dyung has set up a buildbot for this (thanks Doug!), landing shortly.

llvm/include/llvm/IR/SymbolTableListTraits.h
114–118

It has!

llvm/lib/IR/BasicBlock.cpp
225

So done.

llvm/unittests/ADT/IListIteratorBitsTest.cpp
100–101

It's a description of the tests that are below; I think I wrote out the comment to outline what I wanted to test for, then repeated the comments below... I'll just delete it.

jmorse marked an inline comment as done.Oct 17 2023, 7:05 AM
This revision was landed with ongoing or failed builds.Oct 17 2023, 7:27 AM
This revision was automatically updated to reflect the committed changes.