This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef] Don't use instruction referencing for unoptimised functions
ClosedPublic

Authored by jmorse on Aug 23 2021, 2:22 PM.

Details

Summary

InstrRefBasedLDV is marginally slower than VarlocBasedLDV when analysing optimised code -- however, it's much slower when analysing code compiled -O0. Up to 25% slower according to [0], which is too much!

To avoid this: don't use instruction referencing for -O0 functions. In the "pure" case of unoptimised code, this won't really harm the debugging experience because most variables won't have been promoted off the stack, so can't go missing. It becomes more complicated when optimised code is inlined into functions marked optnone; however these are rare, and as -O0 doesn't run many optimisations there should be little damage to the debug experience as a result.

I've taken the opportunity to refactor testing for instruction-referencing into a MachineFunction method, which seems the most appropriate place to put it.

[0] http://llvm-compile-time-tracker.com/compare.php?from=ee8da6369225f47f85e61e1ef2807af6a8677a0d&to=3313f75407bf1f1f4dd787e5233d99cc9e42ba55&stat=instructions

Diff Detail

Event Timeline

jmorse created this revision.Aug 23 2021, 2:22 PM
jmorse requested review of this revision.Aug 23 2021, 2:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2021, 2:22 PM
Orlando accepted this revision.Aug 25 2021, 3:21 AM

LGTM with 2 small inline questions.

llvm/include/llvm/CodeGen/MachineFunction.h
539–540

Nit/question. Am I right in thinking this returns the same value for all MachineFunctions?
If so, then maybe this method should be static? And in the comment I think functions should just become function or be removed. Alternatively, if I'm wrong and this can return a different value for different MachineFunctions (or may do so in the future), then I think the comment should start Returns true if the function's variable....

llvm/test/DebugInfo/X86/instr-ref-opt-levels.ll
11

Should this be O2: DBG_INSTR_REF rather than O2: DBG_VALUE?

This revision is now accepted and ready to land.Aug 25 2021, 3:21 AM
jmorse updated this revision to Diff 368601.Aug 25 2021, 4:02 AM

Ouch, looks like I missed several things: I was intending that functions marked optnone would have instr-ref disabled too, I've added that filter into this patch, and expanded the test case to have an optnone function that always gets a DBG_VALUE.

Additionally there were a bunch of sites where the command-line-option was being tested rather than using this utility method, I'd failed to grep properly for it, oooff. Revised patch modifies all uses of the command line option flag to use the utility method instead. I've also added a RUN line in the test for fast-isel, to check that it behaves the same way as normal isel.