This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Reduce SalvageDebugInfo() functions
ClosedPublic

Authored by chrisjackson on Apr 17 2020, 7:19 AM.

Details

Summary

Having both salvageDebugInfo() and salvageDebugInfoOrMarkUndef() has led to situations when the incorrect function was called. Eliminate the potential for error by combining the two functions. First suggested in D76930.

Now all SalvageDebugInfo() calls will mark undef if the salvage attempt fails. This did not require changes to call sites, except the function name, but some tests resulted in extra debug info (undef) being generated where previously there was none.

Diff Detail

Event Timeline

chrisjackson created this revision.Apr 17 2020, 7:19 AM

Removing a class of bug is always nice.

I have a question for the debug-info reviewers here. Does anyone know what these tests that had to be updated are for? Maybe I'm missing something, but they seem brittle and it's not obvious to me what they are checking for.

llvm/lib/Transforms/Utils/Local.cpp
1631

Is there any value in returning a result at all (i.e. is the return value used anywhere?).

IIRC the only reason the result of salvageDebugInfo was checked was to see if we need to replaceDbgUsesWithUndef (which is now done inside salvageDebugInfo.

aprantl added inline comments.Apr 17 2020, 10:47 AM
llvm/include/llvm/Transforms/Utils/Local.h
359

Should we rename this to updateDebugInfoForDeletedInsn() or some variant of that to make it more clear that it isn't optional to call this method?

Thanks! This lgtm pending outstanding reviewer comments.

I have a question for the debug-info reviewers here. Does anyone know what these tests that had to be updated are for? Maybe I'm missing something, but they seem brittle and it's not obvious to me what they are checking for.

Looks like end-to-end tests for DWARF production under NVPTX. It's probably checking at a more granular level than it needs to. Paging @ABataev, would it be all right to rewrite the test so it pattern-matches against llvm-dwarfdump output?

llvm/include/llvm/Transforms/Utils/Local.h
359

I don't think the current name is unclear (I'm not opposed to changing it, just don't see a strong need to right now).

However, what /does/ seem missing is some way to enforce that this method is called. I like @alok's approach in D70419: wdyt of just making Instruction::eraseFromParent() call Instruction::salvageDebugInfo()? (This would be for a follow-up, my preference would be to consolidate APIs here first.)

vsk added inline comments.Apr 17 2020, 11:23 AM
llvm/include/llvm/Transforms/Utils/Local.h
359

Ah, one issue with this is that the common deletion idiom is: "I.RAUW(replacement); I.eraseFromParent();" -- so if the salageDebugInfo operation occurs in eraseFromParent, in many cases it's too late, because the debug uses are gone.

In D78369#1989363, @vsk wrote:

Thanks! This lgtm pending outstanding reviewer comments.

I have a question for the debug-info reviewers here. Does anyone know what these tests that had to be updated are for? Maybe I'm missing something, but they seem brittle and it's not obvious to me what they are checking for.

Looks like end-to-end tests for DWARF production under NVPTX. It's probably checking at a more granular level than it needs to. Paging @ABataev, would it be all right to rewrite the test so it pattern-matches against llvm-dwarfdump output?

No, llvm-dwarfdump works with object files and we cannot produce it for NVPTX target, it emits only .PTX assembler.

LGTM with comments addressed,

llvm/include/llvm/Transforms/Utils/Local.h
359

I've no opinion on changing the name; for the "RAUW; Erase" deletion idiom I don't think that's a problem, it just means those locations were saved by the RAUW instead of through salvaging. A salvage call in eraseFromParent would be highly useful in catching the more complicated circumstances, for example I believe that during inlining large numbers of instructions are pruned for being unused, without ever being RAUW'd.

chrisjackson marked 4 inline comments as done.May 19 2020, 6:52 AM
chrisjackson added inline comments.
llvm/include/llvm/Transforms/Utils/Local.h
359

Yes this is something I'd like to address too.

359

I'm not sure that a method name is a good way of reflecting its necessity.

Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2020, 6:52 AM

Removed unused return value from SalvageDebugInfo().

dblaikie added inline comments.
llvm/include/llvm/Transforms/Utils/Local.h
359

I'll +1 @aprantl's comment - it sounds like at least from the comment the previous version of this function was "best effort" - if you didn't call it you just got worse debug info. But the new behavior seems like it might break (result in incorrect debug info) if you don't call this function? Is that the case? (if so, then a new name seems justified)

Orlando added inline comments.May 28 2020, 4:39 AM
llvm/include/llvm/Transforms/Utils/Local.h
359

it sounds like at least from the comment the previous version of this
function was "best effort" - if you didn't call it you just got worse debug
info.

This is only true if replaceDbgUsesWithUndef was being called where
appropriate. With D80264 we do actually get this for free in a lot of cases,
but there are still times where we want to undef a dbg.value without deleting
the instruction. E.g. When sinking an instruction with a corresponding
dbg.value, we want to leave behind a salvage-or-undef, and sink a clone
of the original dbg.value to the new instruction position.

dblaikie added inline comments.May 28 2020, 6:47 PM
llvm/include/llvm/Transforms/Utils/Local.h
359

it sounds like at least from the comment the previous version of this

function was "best effort" - if you didn't call it you just got worse debug
info.

This is only true if replaceDbgUsesWithUndef was being called where

appropriate.

Sorry, not sure I understand - you mean previously you had to call one of either replaceDbgUsesWithUndef (not anymore, since D80264, now that's implicit) or salvageDebugInfo - and now you have to always call salvageDebugInfo ?

(maybe I shuold just bow out of this review - I haven't gotten very involved in LLVM's debug info variable location tracking, so I don't have a lot of context for this - just wanted to express some agreement with @aprantl's perspective there)

Orlando added inline comments.May 29 2020, 12:31 AM
llvm/include/llvm/Transforms/Utils/Local.h
359

Ah sorry for being unclear, I hope this helps. I don't have a strong opinion on the name change.

AFAIK it is correct to call replaceDbgUsesWithUndef if salvageDebugInfo failed (returns false), and not doing so can lead to wrong debug-info by allowing previous dbg.value definitions to run on for too long. This patch changes salvageDebugInfo to call replaceDbgUsesWithUndef itself if it fails to salvage. Because replaceDbgUsesWithUndef is now called automatically when we delete an instruction (D80264), not calling salvageDebugInfo just reduces coverage, but doesn't produce incorrect debug-info when deleting instructions.

But the new behavior seems like it might break (result in incorrect debug info) if you don't call this function? Is that the case? (if so, then a new name seems justified)

So I think the answer to this is no, now that we have D80264.

LGTM. There are two other text only LGTMs on this patch and AFAICT the only blocker is the (inline) discussion on whether salvageDebugInfo needs renaming, but there doesn't seem to be a consensus. Does anyone have a strong opinion on this subject?

I feel like everyone agrees that the patch is good, but I don't want to press the button and to have inadvertently shut someone down.

vsk accepted this revision.Jun 1 2020, 11:57 AM

This looks good to me. We should consolidate the salvageDebugInfo* APIs. I think the conversation about renaming the API is a bit moot post-D80264 (afaict the motivation for the rename was that /not/ calling the API could result in invalid debug info -- that should be fixed now).

llvm/include/llvm/Transforms/Utils/Local.h
356

Maybe 's/mark it/change its debug uses to/'?

This revision is now accepted and ready to land.Jun 1 2020, 11:57 AM
chrisjackson closed this revision.Jun 10 2020, 6:50 AM

This revision was closed with commit c6c65164. I'm uncertain why the commit's 'differential revision' tag failed to trigger the autoclose.