Page MenuHomePhabricator

[SimplifyCFG] Keep !dgb metadata of moved instruction, if they match.
ClosedPublic

Authored by fhahn on Dec 21 2020, 12:59 PM.

Details

Summary

Currently SimplifyCFG drops the debug locations of 'bonus' instructions.
Such instructions are moved before the first branch. The reason for the
current behavior is that this could lead to surprising debug stepping,
if the block that's folded is dead.

In case the first branch and the instructions to be folded have the same
debug location, this shouldn't be an issue and we can keep the debug
location.

Diff Detail

Event Timeline

fhahn created this revision.Dec 21 2020, 12:59 PM
fhahn requested review of this revision.Dec 21 2020, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2020, 12:59 PM
vsk added a comment.Sun, Jan 3, 5:06 PM

In its current form, the rule for preserving debug locs doesn't seem to apply here (https://llvm.org/docs/HowToUpdateDebugInfo.html#when-to-preserve-an-instruction-location). Imho we ought to revise the guidance there before proceeding with this patch (e.g. by adding some carveout for when the destination debug loc is the same). If you can share, it might also help to have a little context - unless there's some intervening optimization, the dwarf generator's location cascade should kick in in this situation?

fhahn added a comment.Mon, Jan 4, 3:31 AM
In D93662#2476793, @vsk wrote:

In its current form, the rule for preserving debug locs doesn't seem to apply here (https://llvm.org/docs/HowToUpdateDebugInfo.html#when-to-preserve-an-instruction-location). Imho we ought to revise the guidance there before proceeding with this patch (e.g. by adding some carveout for when the destination debug loc is the same). If you can share, it might also help to have a little context - unless there's some intervening optimization, the dwarf generator's location cascade should kick in in this situation?

The use case I am looking at is optimization remarks, which use !dbg to tie remarks back to the high-level source code. I am working on remarks that use !dbg to group together instructions that originated from the same place in the original source. For that, it is very convenient if all instructions have accurate !dbg metadata. With what you said earlier, it might be good to also scan forwards/backwards to the next instruction with !dbg and use that.

I don't know much about how !dbg is actually used by the debuggers, but it seems like a rule like if an instruction is moved to a new insert point and the instruction at the insert point has the same debug location as the moved instruction the location can be preserved on the moved instruction should not regress the debug experience. And in some cases, it might be beneficial, e.g. in case later optimization move the same instructions around again. What do you think?

jmorse added a comment.Tue, Jan 5, 6:47 AM

This improvement looks good and preserves more source locations. Awkwardly, as @vsk says it's quite different from the other documented rules, because it relies on the context of where the instructions are going to be placed.

In terms of wording, how about adding an additional clause:

A transformation should preserve the debug location of an instruction [...] if the destination block already contains an instruction with an identical debug location

Which is slightly broader than your wording @fhahn, but IMHO gets across the idea that the location shouldn't be preserved if it's "new" to a block. Opinions?

vsk added a comment.Tue, Jan 5, 9:11 AM

This improvement looks good and preserves more source locations. Awkwardly, as @vsk says it's quite different from the other documented rules, because it relies on the context of where the instructions are going to be placed.

+1

In terms of wording, how about adding an additional clause:

A transformation should preserve the debug location of an instruction [...] if the destination block already contains an instruction with an identical debug location

Which is slightly broader than your wording @fhahn, but IMHO gets across the idea that the location shouldn't be preserved if it's "new" to a block. Opinions?

I think that sounds reasonable. My initial thought was that the broader language might invite folks to consider scanning the destination block for a matching location: there's nothing wrong with doing that in principle, but imho it ought to be discouraged for performance reasons.

fhahn updated this revision to Diff 315142.Thu, Jan 7, 7:45 AM
In D93662#2479778, @vsk wrote:

In terms of wording, how about adding an additional clause:

A transformation should preserve the debug location of an instruction [...] if the destination block already contains an instruction with an identical debug location

Which is slightly broader than your wording @fhahn, but IMHO gets across the idea that the location shouldn't be preserved if it's "new" to a block. Opinions?

I think that sounds reasonable. My initial thought was that the broader language might invite folks to consider scanning the destination block for a matching location: there's nothing wrong with doing that in principle, but imho it ought to be discouraged for performance reasons.

I updated the doc with the suggested wording. Not sure if it is worth adding a clause saying to should only be done when it can be cheaply determined that the location is not new to the block?

vsk accepted this revision.Fri, Jan 8, 11:56 AM

Looks good to me as-is. Thanks!

This revision is now accepted and ready to land.Fri, Jan 8, 11:56 AM
fhahn added a comment.Sat, Jan 9, 11:17 AM
In D93662#2487498, @vsk wrote:

Looks good to me as-is. Thanks!

Great, thanks everyone!