Page MenuHomePhabricator

[MachineOutliner] Do not outline debug instructions
ClosedPublic

Authored by chill on Oct 15 2020, 10:10 AM.

Details

Summary

The debug location is removed from any outlined instruction. This causes the
MachineVerifier to crash on outlined DBG_VALUE instructions.

Then, debug instructions are "invisible" to the outliner, that is, two ranges of
instructions from different functions are considered identical if the only
difference is debug instructions (is that right?). Since a debug instruction
from one function is unlikely to provide sensible debug information about all
functions, sharing an outlined sequence, this patch just removes debug
instructions from the outlined functions.

Diff Detail

Event Timeline

chill created this revision.Oct 15 2020, 10:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2020, 10:10 AM
chill requested review of this revision.Oct 15 2020, 10:10 AM

(Adding debug-info),

If I understand correctly, the outliner "commons" code between different functions, yes? In that case, then as you say it's definitely correct to drop variable locations, they'll be especially broken seeing how the outlined function is given a separate DISubprogram.

This seems correct to me. Outlined functions should not contain debug instructions.

Only thing is that it'd be a lot nicer to have a MIR test here. That would make it much clearer what the testcase is actually doing.

llvm/test/CodeGen/ARM/machine-outliner-remove-debug-instr.ll
1 ↗(On Diff #298411)

Can this be a MIR test instead?

chill updated this revision to Diff 300294.Oct 23 2020, 8:00 AM
chill edited the summary of this revision. (Show Details)
chill marked an inline comment as done.
paquette added inline comments.Nov 2 2020, 9:09 AM
llvm/test/CodeGen/ARM/machine-outliner-remove-debug-instr.mir
2
  • You can pull the triple into the runline
  • Don't need -- for all of the flags
15

Remove comments in the IR

40

I don't think you need to include the actual IR here?

This should work:

define i32 @h() { ret void }
define i32 @f() { ret void }
define i32 @g() { ret void }
...

If it doesn't work, then the MIR is referencing the IR in some way. It may be possible to just drop the references to the IR in the MIR in order to get the simplest test possible.

It would be ideal to end up with something like this:

# RUN: ...
--- |
define i32 @h() { ret void }
define i32 @f() { ret void }
define i32 @g() { ret void }
...
---
name:            h
... MIR test ...

Or at least as close to the above as possible.

171

I don't think most of the entries here are necessary.

I think all you need is something like

name:            h
tracksRegLiveness: true
body:             |

(see: llvm/test/CodeGen/AArch64/machine-outliner-calls.mir for an example)

200

I think you can drop the references to the IR here.

(Judging by llvm/test/CodeGen/AArch64/ldst-opt-mte-with-dbg.mir)

chill updated this revision to Diff 302543.Nov 3 2020, 4:36 AM
chill marked 5 inline comments as done.

Can you reduce the contents of f, g, and h so that you only have

  • The instructions which are outlined
  • Debug instructions which should not be outlined

This will make the testcase a lot easier for people reading it to understand what's going on. It will also make it less likely for it to fail because of unrelated changes.

llvm/test/CodeGen/ARM/machine-outliner-remove-debug-instr.mir
15

This part of the IR isn't necessary

16

You can remove this part too

39

Is this function outlined from at all? Can it just be removed?

chill updated this revision to Diff 302844.Nov 4 2020, 7:44 AM
chill marked 3 inline comments as done.

That should be the minimum number of instructions now, one less and the outlining does not pass the cost threshold.

paquette accepted this revision.Nov 4 2020, 2:03 PM

LGTM!

This revision is now accepted and ready to land.Nov 4 2020, 2:03 PM
This revision was automatically updated to reflect the committed changes.