PSA: Instruction-constructors changing to iterator-only insertion

IR & Optimizations
,
1

This is a heads-up about some important changes we (Sony) are making to the Instruction-creation APIs; the work for this was done by @jmorse, and the PSA below was written by him, but he’s on holiday for a while so I’ll be handling the attempt to land the work:

Hi all,

FYI, we’re planning on removing a whole host of Instruction constructors and Create methods that accept Instruction pointers, in favour of taking an iterator. This is likely to break a lot of downstream code in a way that seems spurious, so I thought I’d explain why it’s worth the disruption.

Briefly: all subclasses of Instruction have two flavours of constructors and Create methods right now, such as:

static BinaryOperator *CreateNot(Value *Op, const Twine &Name = "", Instruction *InsertBefore = nullptr);
static BinaryOperator *CreateNot(Value *Op, const Twine &Name,  BasicBlock *InsertAtEnd);

These take a block to insert into, or an instruction. They all have a defaultable-to-nullptr flavour that doesn’t insert anywhere too. We’ve recently added versions of each that take a BasicBlock::iterator instead of an Instruction pointer and converted almost all of LLVM to use the iterator-flavour if needed. The plan is to soon (weeks) delete the Instruction pointer taking flavour (and let the block-taking version default to nullptr):

static BinaryOperator *CreateNot(Value *Op, const Twine &Name = "",  BasicBlock *InsertAtEnd = nullptr);
static BinaryOperator *CreateNot(Value *Op, const Twine &Name, BasicBlock::iterator InsertBefore);

We’re doing this so that the correctness of debug-info updating is in the type system. As discussed in my proposal last year [0], we can eliminate debug intrinsics and maintain debugging information without getting in the way of optimisation passes, so long as we can identify whether passes intend to insert something at the beginning of a block. That’s achieved by putting a single bit in BasicBlock::iterator to signal if it was created by getFirstInsertionPt / getFirstNonPHI, and ensuring passes use those iterators to insert instructions. Once this is done you’ll never have to see a dbg.value intrinsic again, and the type system will encourage the correct identification of the start of blocks.

We’ve already enabled intrinsic-less debug-info mode and observed compile-time speedups (it’s usually proportionate to the amount of inlining) and a large reduction in debug-info having an effect on codegen decisions in our downstream tests. These constructor-changes are the final step needed to guarantee correctness in the future (there will be subtle regressions and off-by-ones otherwise). Thank you for your understanding!

~

The patch taking away the relevant constructors can be found here [1] if you want to make the necessary changes ahead-of-time. 95% of the insertion sites require trivial changes that are basically just changing a spelling. When making changes to a call-site that inserts an instruction using an instruction pointer:

  • If the insertion-position identified because of what the position does, i.e. it computes a particular value, it’s a particular kind of instruction, it’s a user of a particular value… then just call getIterator() on the insert-site instruction pointer.
  • If the insertion-position is identified because of /where/ it is in the block, I would encourage using getIterator() and std::next / std::prev to find that position, but you could use getNextNode / getPrevNode and getIterator too without worry,
  • For any situation where the position might have been sourced from BasicBlock::begin, or getFirstNonPHI, or getFirstInsertionPt, you need to store that iterator and use it for the insertion position.

[0] [RFC] Instruction API changes needed to eliminate debug intrinsics from IR
[1] [RemoveDIs][NFC] Remove Instruction constructors that insert prior to Instructions by SLTozer · Pull Request #85980 · llvm/llvm-project · GitHub

3 Likes
2

Doesn’t seem to be any mention of deprecating these functions first? Or are they already marked deprecated? Or some reason they can’t be marked deprecated?

3 Likes
3

+1

Deprecating first gives downstream folks with automergers a much easier time, since it de-couples merge resolution from fixing all the issues, which might have different experts in different part of the codebase.

2 Likes
4

Deprecating the functions first seems sensible, and generally anything that can make the change easier for downstream folk to navigate is good. They haven’t been marked deprecated yet because this issue came up relatively late in the course of RemoveDIs, and our attention has been focused on the implementation of the feature.

With that said, how long do you think the functions should be kept in a deprecated state prior to being deleted? The reason for deleting them is that there are some subtle errors that can occur if they’re used in a way that drops the extra iterator bit when it should be set, so ideally we’d have downstream users migrate to the new functions before too long.

5

When they are deprecated (as in “marked with the relevant macro”), any new use upstream would trigger a warning, and since some of use are building with Werror we’d notice immediately.

So basically the way I see it: the advantage of marking them deprecated is that it should put less pressure to remove them quickly (assuming having them here does not prevent other work you’re doing) since we guarantee they will be unused in the project.

1 Like
6

As for “how long” there is an argument for waiting until after the next release branch (which ought to be in July). That way, downstream projects that update only once per release will have the opportunity to fix themselves up in a non-emergency way.

1 Like
7

I’m sympathetic to the argument of waiting for a release, since that seems to be the usual way that the deprecation process takes place (deprecate for the next release, remove the release after), but it also needs to be taken into account that certain uses of the deprecated functions can cause crashes right now, which means leaving them deprecated-but-present in the next release might result in problems for downstream users that don’t update their uses by then.

I don’t know whether that’s a justification to jump straight to deleting those functions - as long as the functions are marked deprecated then downstream users may be considered to have been warned.

1 Like
8

Why do they cause crashes right now? Did some changes for intrinsic-less debuginfo get a bit ahead of API compatibility? Could we unwind some of that so these can be deprecated/migrated first?

9

I missed a step here - you’re suggesting deprecating but leaving them present in the coming/next release would be bad. But then suggesting that if we mark them deprecated, users have been warned.

Have these functions already been deprecated in a prior release?

10

The short summary of the error (discussed on discourse and github) is that inserting PHIs with an Instruction as the insert position rather than an iterator can result in them being inserted after debug records that appear between the last PHI and the first instruction, which is a verifier error (when we convert back to intrinsics at least). There is an assertion for this, so any instances of the error should fail fast with a trivial fix (use the InsertAtHead bit, ideally by inserting at BasicBlock::getFirstNonPHIIt()). Outside of the assertion failure, consequences of using the wrong insertion method are limited to instructions being inserted after a block of debug records when they should be inserted before.

I missed a step here

I was just stating the case as I saw it for both sides (removing or deprecating). I believe that the right action would be to mark the functions as deprecated for the next release, with documentation that clearly explains the problem and how to avoid it.

11

It seems to me that implementation of the function could handle the special case and move the insertion point before the debug intrinsics?

12

Yes, I think that’s possible in the context of us deprecating the problematic constructors instead of deleting them. They still need to be deprecated and eventually removed since they can introduce off-by-1 debug info errors, but at least for the deprecated period we can prevent the actual crashes from occurring; it’s something I’ll be testing and potentially submitting soon.

1 Like
13

Sounds good to me - ensuring the APIs work today, deprecating them for a release, then removing them.

Not always what we do for LLVM APIs (generally not much in the way of guarantees of stability in LLVM’s C++ APIs), but the builder APIs in particular (& similarly we often do this for APIs in Support/ADT) they’ve got a fair bit of external usage exposure, so a more cautious transition is suitable.

SGTM then - are there other questions to resolve in this thread, then? Or probably just hash out the release note/documentation wording on how to appropriately warn/inform/educate people on how to handle the transition in some PRs?

3 Likes
14

SGTM then - are there other questions to resolve in this thread, then? Or probably just hash out the release note/documentation wording on how to appropriately warn/inform/educate people on how to handle the transition in some PRs?

Nothing that needs to be covered I think - mostly this thread is an early announcement of what we intend to do, and a chance for people to comment and suggest changes (which has clearly been beneficial so far!). For the release notes and documentation, I assume people will contribute during review of the pull requests that add them, but obviously if anyone has any downstream circumstances that might make these API changes difficult to deal with then it’d be good to hear about them sooner. Once we’ve opened the deprecation patch for review we’ll also post in the announcements category to make sure that people get a fair chance to see it and make any other suggestions.

Although as one extra question on that note - just in case anyone here has opinions - we’ve also been making changes to the DIBuilder in the LLVM C API; we’ve already merged two patches (1, 2) but haven’t gotten a lot of feedback from consumers of the API - there’s no code owner for “C”, but if anyone here uses the C API downstream then it’d be helpful to know if the updated APIs work nicely with their downstream changes, or if there’s anything that looks off about the changes.

2 Likes
15

For any situation where the position might have been sourced from BasicBlock::begin, or getFirstNonPHI, or getFirstInsertionPt, you need to store that iterator and use it for the insertion position

What goes wrong when this invariant is not followed?

I’m particular it seems quite likely that various places will conjure something that compiles without crawling from begin to get to the insertion point. I don’t see how the C++ type system would prevent that. Maybe we could have a separate iterator-from-begin type and lock down some of the API to require that variant, but most of llvm is dyn_cast style which seems at odds with compile time constraints.

If that’s a compiler compile time failure, great. If it’s caught by report_fatal_error at compiler run time, maybe under asserts builds, also good. If it’s silently bad debug info, sad but tolerable. If it’s silent miscompilation of executable code, we should find something to take the sharp edge off.

Thanks!

16

Yeah, I don’t think it’s feasible to completely prevent incorrect insertions here, particularly because we don’t even necessarily want to prevent iterator-instruction-iterator roundtrips from being valid insert positions; for example, if we want to iterate through a basic block to find an instruction of interest, perform some operation on that instruction, and then insert a new instruction behind it.

The current state is that instruction iterators now contain more information than just the instruction it points to, and that extra bit of information is relevant for debug info correctness when inserting instructions, so this change is attempting to guide people towards correct usage by removing footguns rather than creating totally inviolable compile-time constraints.

For most instructions, inserting incorrectly means off-by-1 debug variable ranges - so debug values will become live or dead at the wrong place. The PHI insertion problem can be worked around for the foreseeable future; currently it’s a fatal error with assertions, but it can probably be silently fixed. Neither case can result in a miscompile, only bad debug info.

17

On our end, a few weeks would be enough for this sort of deprecation, I think.

18

Just an update here - I’ve opened a separate RFC to discuss a solution to the problem of preventing incorrect insertions: [RFC] Rewriting the instruction insertion interface

The proposed rewrite is not strictly required and could be introduced on a different time scale to this change, but the goal is to make the constraints discussed in this PSA easier to maintain by making the interface for inserting instructions more explicit about the intended position, without requiring particular knowledge of debug records.