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

Mon Jun 26 08:14:53 PDT 2023

jmorse created this revision.
jmorse added reviewers: StephenTozer, Orlando, probinson, dexonsmith.
Herald added a subscriber: hiraditya.
Herald added a project: All.
jmorse requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153777

Files:
  llvm/CMakeLists.txt
  llvm/cmake/modules/HandleLLVMOptions.cmake
  llvm/include/llvm/ADT/ilist_iterator.h
  llvm/include/llvm/ADT/ilist_node.h
  llvm/include/llvm/ADT/ilist_node_options.h
  llvm/include/llvm/ADT/simple_ilist.h
  llvm/include/llvm/IR/BasicBlock.h
  llvm/include/llvm/IR/GlobalAlias.h
  llvm/include/llvm/IR/GlobalIFunc.h
  llvm/include/llvm/IR/GlobalVariable.h
  llvm/include/llvm/IR/Instruction.h
  llvm/include/llvm/IR/Instructions.h
  llvm/include/llvm/IR/SymbolTableListTraits.h
  llvm/include/llvm/IR/ValueSymbolTable.h
  llvm/lib/IR/BasicBlock.cpp
  llvm/lib/IR/Instruction.cpp
  llvm/lib/IR/Instructions.cpp
  llvm/lib/IR/SymbolTableListTraitsImpl.h
  llvm/unittests/ADT/CMakeLists.txt
  llvm/unittests/ADT/IListIteratorBitsTest.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D153777.534562.patch
Type: text/x-patch
Size: 35860 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230626/77b928fa/attachment.bin>