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.