Page MenuHomePhabricator

[DebugInfo] Remove now un-necessary logic from HoistThenElseCodeToIf

Authored by jmorse on Dec 4 2018, 7:51 AM.



tl;dr: minor cleanup of line number computation to remove un-necessary complexity and avoid keeping dead line numbers alive.

This is a follow-up to D54997, where we saw that applyMergedLocation was required for correctly calculating debug locations when hoisting terminators. Using applyMergedLocation is correct, but makes the code necessary for fixing PR39187 [0,1] redundant: we now cannot leak debug locations from the hoisted blocks into the parent block via the terminator.

IMHO, when converting phis to selects in HoistThenElseCodeToIf we should remove the logic looking for prior non-debug instructions, and just inherit from the (merged) terminator location. The net effect is that code looking like this:

%10 = insn1...   # Line 1
%11 = insn2...   # Line 2
%12 = select...   # Line 2 (hoisted phi converted to select)
%13 = invoke @something to label %blah unwind label %fail   # Line 0

where the select inherited the line number from the insn above, will instead become:

%10 = insn1...   # Line 1
%11 = insn2...   # Line 2
%12 = select...   # Line 0 (hoisted phi converted to select)
%13 = invoke @something to label %blah unwind label %fail   # Line 0

where the line number comes from the hoisted terminator. There are two benefits:

  • Simpler code in HoistThenElseCodeToIf
  • Line numbers that get optimised away won't be kept alive through selects that appeared below them

I've adapted the original regression test to check for selects introduced by HoistThenElseCodeToIf getting an unknown location. The main point of the original PR was that they shouldn't get the line number from the hoisted terminator, which this behaviour honours.

This leaves the "getPrevNonDebugInstruction" methods without any users, but they seem like useful facilities to keep around.


Diff Detail


Event Timeline

jmorse created this revision.Dec 4 2018, 7:51 AM
aprantl added inline comments.Dec 4 2018, 9:18 AM
1388 ↗(On Diff #176629)

// PHIs created below will adopt NT's merged DebugLoc.

jmorse updated this revision to Diff 176669.Dec 4 2018, 10:14 AM

Revise comment

vsk accepted this revision.Dec 7 2018, 8:50 AM
vsk added a subscriber: CarlosAlbertoEnciso.

LGTM. @CarlosAlbertoEnciso, any thoughts on this?

This revision is now accepted and ready to land.Dec 7 2018, 8:50 AM
In D55272#1323317, @vsk wrote:

LGTM. @CarlosAlbertoEnciso, any thoughts on this?

FYI Carlos is away until the second week of January.

NB, speaking directly to Carlos yesterday he approved of the cleanup. I'll likely land this today or Monday.

This revision was automatically updated to reflect the committed changes.