Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[NFC][DebugInfo][RemoveDIs] Use iterators over instruction pointers when using IRBuilder in various passes

Authored by jmorse on Jun 8 2023, 2:18 PM.



This patch adds a two-argument SetInsertPoint method to IRBuilder that takes a block/iterator instead of an instruction. The patch also updates a variety of call sites to pass around iterators instead of instruction pointers. The motivating reason for doing this is given here [0], we'd like to pass around more information about the position of debug-info in the iterator object. That necessitates passing iterators around most of the time.

The set of changes in this patch are all NFC, and cover all uses of IRBuilder that are involved in stage2clang and a couple of other large C++ codebases I have to hand. It's not all the uses of SetInsertPoint, but should give an idea of how invasive the changes we want to make are. (In my opinion: not invasive).

No tests because this is all NFC; I do have various LLVM-IR inputs that are sensitive to these changes, but the exact test strategy and what order to land patches in is up for discussion. I'll elaborate in D151419, and not add reviewers here yet.


Diff Detail

Event Timeline

jmorse created this revision.Jun 8 2023, 2:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2023, 2:18 PM
Herald added subscribers: pmatos, asb, foad and 11 others. · View Herald Transcript
jmorse requested review of this revision.Jun 8 2023, 2:18 PM

(Herald added reviewers; removing to signal that I'm not rreeaaally request review on this just yet, although it is "reviewable").

jmorse retitled this revision from [DebugInfo][RemoveDIs] Use iterators over instruction pointers when using IRBuilder in various passes to [NFC][DebugInfo][RemoveDIs] Use iterators over instruction pointers when using IRBuilder in various passes.

The changes from getFirstNonPHI to getFirstInsertionPt make me worry that this isn't NFC as it looks like the latter skips past EH instructions.


IIUC front and begin are equivalent (at the moment). What is the purpose of this change? If it's set-up for a future change in semantics of these functions it could be worth explaining the change in the commit message IMO.

(This pattern is repeated in multiple places)


Why change from getFirstNonPHI to getFirstInsertionPt? They are not equivalent - the latter also skips past EH instructions.

(This pattern is repeated in multiple places)


nit: Possibly redundant ctor (insertion point set below now) - possibly clearer to use the Context & one (with other default params)?

jmorse marked an inline comment as done.Jul 25 2023, 10:08 AM

Good point about the EH_LABEL situation, that was indeed going to mess with things. Revising in a moment.

Will look at the extra constructor situation shortly.


The purpose is signalling to SetInsertPoint that the location being set is at the start of the block, before any debug info -- if we pass a pure Instruction * then there's no way to store that information, wheras by passing an iterator, we can put a flag in the iterator to store this fact. (See: D153777).

As you say it's down to the semantics of the functions changing; I plan on linking to the general RFC thinymajig and having a terse summary.


I've just been running into this elsewhere ._., all our code samples have exceptions disabled so we haven't hit this in the past. There is indeed a semantic difference between the two, so I'm revising these away and adding a getFirstNonPHIIt method that returns an iterator, again to signal whether a position is in front of debug-info or not. The relevant flag will be set at D153777

The result should be that when you get this:

    <--- debug data

Then getFirstNonPHIIt will return an iterator to the Add with the head-bit set, while if you have this:

    <--- debug data

You'll get an iterator to the EH_LABEL, with the head bit set, but the bit won't have any effect because of the invariant that we don't insert debug-info before PHI's / LABEL's.

jmorse updated this revision to Diff 544022.Jul 25 2023, 10:10 AM
jmorse marked an inline comment as done.

Switch getFirstNonPHI to getNonFirstPHIIt to transmit some debug-info position data, when the time comes.

jmorse updated this revision to Diff 555704.Sep 4 2023, 4:24 AM

Use insertion-point setter in IRBuilders constructor rather than calling SetInsertionPoint again.

Orlando accepted this revision.Sep 5 2023, 1:12 AM

LGTM (with a few nits)


nit: please clang-format. Also, any reason that the const version is implemented in the .cpp file while this one is in the header?


nit: I don't think we need the new braces here?

This revision is now accepted and ready to land.Sep 5 2023, 1:12 AM
jmorse updated this revision to Diff 556158.Sep 7 2023, 8:24 AM

clang-formatted, inline fix suggestions and rebasing. In the rebase, I've lost two hunks in code that's since been refactored (LCSSA and one of many in LoopVectorise). Either that code has been deleted and we don't have to think about it any more, or it's code that will show up in the future as causing a difference between RemoveDIs / dbg.value mode, and it'll be addressed in a follow up patch.

Not landing this patch now as it depends on the parent's addition of getStableDebugLoc. I'll strip the questioned parts of that patch out and handwave about landing it.

jmorse added inline comments.Sep 7 2023, 8:46 AM

In the cpp file -> I'm following the convention of the immediately preceeding (in this hunk) getFirstNonPHI which does the same thing. I don't know why getFirstNonPHI was written like that, but it makes sense to copy it here IMO.

jmorse updated this revision to Diff 556178.Sep 7 2023, 10:42 AM

(Now attaching this patch to the right review...) Actually, the majority of the base patch is questioned, so I've pulled getStableDebugLoc into this one, if that's alright. (It's a debug-intrinsic-skipping getDebugLoc).

Mucked around with this further and discovered that some tests are now sensitive to the getStableDebugLoc change -- I think the test fixes as a result are fairly obvious but they could do with reviewing, please see D159485 too.

This revision was landed with ongoing or failed builds.Sep 11 2023, 12:02 PM
This revision was automatically updated to reflect the committed changes.