Page MenuHomePhabricator

Add MIR-level debugify with only locations support for now
ClosedPublic

Authored by dsanders on Apr 3 2020, 5:35 PM.

Details

Summary

Re-used the IR-level debugify for the most part. The MIR-level code then
adds locations to the MachineInstrs afterwards based on the LLVM-IR debug
info.

It's worth mentioning that the resulting locations make little sense as
the range of line numbers used in a Function at the MIR level exceeds that
of the equivelent IR level function. As such, MachineInstrs can appear to
originate from outside the subprogram scope (and from other subprogram
scopes). However, it doesn't seem worth worrying about as the source is
imaginary anyway.

There's a few high level goals this pass works towards:

  • We should be able to debugify our .ll/.mir in the lit tests without changing the checks and still pass them. I.e. Debug info should not change codegen. Combining this with a strip-debug pass should enable this. The main issue I ran into without the strip-debug pass was instructions with MMO's and checks on both the instruction and the MMO as the debug-location is between them. I currently have a simple hack in the MIRPrinter to resolve that but the more general solution is a proper strip-debug pass.
  • We should be able to test that GlobalISel does not lose debug info. I recently found that the legalizer can be unexpectedly lossy in seemingly simple cases (e.g. expanding one instr into many). I have a verifier (will be posted separately) that can be integrated with passes that use the observer interface and will catch location loss (it does not verify correctness, just that there's zero lossage). It is a little conservative as the line-0 locations that arise from conflicts do not track the conflicting locations but it can still catch a fair bit.

Depends on D77439, D77438

Diff Detail

Event Timeline

dsanders created this revision.Apr 3 2020, 5:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 5:35 PM

Cool! Vedant should probably look this over, since he implemented the IR pass.

vsk added a comment.Apr 6 2020, 12:27 PM

Awesome. Agreed that a proper -strip-debug pass is sorely needed (one that strips out the intrinsics/meta-instructions, dead prototypes, and unused metadata).

This looks good overall but needs a test.

llvm/include/llvm/Transforms/Utils/Debugify.h
22

Could you please add doxygen for the parameters?

llvm/lib/CodeGen/MachineDebugify.cpp
32

Is MF.dump() needed?

46

Yes, this is fine.

dsanders marked 3 inline comments as done.Apr 6 2020, 4:17 PM
dsanders added inline comments.
llvm/include/llvm/Transforms/Utils/Debugify.h
22

Sure

llvm/lib/CodeGen/MachineDebugify.cpp
32

Oops. I meant to remove that. It's how I confirmed I had the boilerplate working before I started adding the code.

dsanders updated this revision to Diff 255539.Apr 6 2020, 4:40 PM

Add doxygen comment
Remove MF.dump()

dsanders marked 2 inline comments as done.Apr 6 2020, 4:40 PM
vsk added a comment.Apr 7 2020, 11:06 AM

I realize this functionality will be exercised in D77576, but still think it'd be worthwhile to add a standalone test for this.

dsanders updated this revision to Diff 255774.Apr 7 2020, 12:39 PM

Add test case

vsk accepted this revision.Apr 7 2020, 2:00 PM

Thanks, lgtm!

Later on, I think we'll want to insert DBG_VALUE insts as well, but this looks very useful as-is.

This revision is now accepted and ready to land.Apr 7 2020, 2:00 PM
In D77446#1967893, @vsk wrote:

Thanks, lgtm!

Later on, I think we'll want to insert DBG_VALUE insts as well, but this looks very useful as-is.

Yep, I'll need to find all my DBG_VALUE-affects-CodeGen bugs :-)

This revision was automatically updated to reflect the committed changes.

Thanks for this work! I've missed this, since it was not marked as 'debug-info' project.

I think we should document this at https://llvm.org/docs/SourceLevelDebugging.html#testing-debug-info-preservation-in-optimizations.

Thanks for this work! I've missed this, since it was not marked as 'debug-info' project.

I think we should document this at https://llvm.org/docs/SourceLevelDebugging.html#testing-debug-info-preservation-in-optimizations.

The D80052 will address this comment.

Thanks for this work! I've missed this, since it was not marked as 'debug-info' project.

I think we should document this at https://llvm.org/docs/SourceLevelDebugging.html#testing-debug-info-preservation-in-optimizations.

The D80052 will address this comment.

Good point. There's also the LostDebugLocObserver for detecting location loss during backend passes which I've mentioned on that review. If Vedant adds a section for it I can fill in some detail on how to use it afterwards.

Thanks

ychen added a subscriber: ychen.Jul 21 2020, 5:23 PM

Any reason this is a module pass instead of a machine pass?