Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Add DBG_VALUE support to the linear DAG scheduler
ClosedPublic

Authored by jcranmer on Feb 7 2018, 10:06 AM.

Details

Summary

I noticed that the fast/linear DAG scheduler doesn't lower DBG_VALUEs except for function entry nodes. This copies what appears to be the relevant code, and it does seem to work for most of the simple testing I've done.

Diff Detail

Repository
rL LLVM

Event Timeline

jcranmer created this revision.Feb 7 2018, 10:06 AM

Seems reasonable. Adding a couple of folks who know a bit more about debug info than I do to approve this.

vsk added a subscriber: vsk.Feb 8 2018, 9:56 AM
vsk added a subscriber: debug-info.Feb 9 2018, 5:37 PM

Thanks for working on this.

lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp
791

Nit: could you used a range-for loop here and run clang-format over the diff?

test/CodeGen/Generic/linear-dbg-value.ll
29

I'm not able to follow what's changed based on these check lines. Could you attach llc's pre-patch/post-patch output, so we can see how the output changes?

probinson added inline comments.
lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp
797

Don't put braces around a single-line body.
Could compact these 4 lines to 2:

if (auto *DbgMI = Emitter.EmitDbgValue(DVs[i], VRBaseMap))
  BB->insert(InsertPos, DbgMI);

Before patch:

After patch:

Effectively, before this patch, the only dbg.value entries that get added are those that have to do with function arguments. The ones in the middle of the function are completely ignored.

jcranmer updated this revision to Diff 133781.Feb 10 2018, 6:10 PM
jcranmer marked 3 inline comments as done.
aprantl added inline comments.Feb 12 2018, 9:14 AM
test/CodeGen/Generic/linear-dbg-value.ll
46

please remove all non-essential attributes. (everything in quotes)

vsk added inline comments.Feb 12 2018, 10:27 AM
test/CodeGen/Generic/linear-dbg-value.ll
29

Thanks! Could you also check that the second dbg.value() for %add is inserted right after the first one?

jcranmer updated this revision to Diff 134158.Feb 13 2018, 8:16 PM
jcranmer marked 2 inline comments as done.

Hopefully this should be the last version.

This is in line with how we handle debug values in ScheduleDAGSDNodes::EmitSchedule (see ProcessSDDbgValues).

I don't see any issues with this patch but am not familiar enough with ScheduleDAGFast to give an explicit lgtm. I'll CC some folks who might be able to.

aprantl added inline comments.Feb 20 2018, 5:17 PM
test/CodeGen/Generic/linear-dbg-value.ll
30

It would be better to write this as

CHECK: DBG_VALUE ![[X:[0-9]+]], ![[EXPR:[0-9]+]]
CHECK: [[EXPR]] = !DIExpression()
...

etc. Hard-coding the metadata numbering is just asking for trouble :-)

aprantl accepted this revision.Feb 20 2018, 5:17 PM

Seems fine otherwise, thanks!

This revision is now accepted and ready to land.Feb 20 2018, 5:17 PM

How does this patch get checked in? I don't have commit privileges, so far as I know.

I can commit this for you.

aprantl requested changes to this revision.Mar 2 2018, 2:36 PM

There's something weird with the test output:

bb.3.for.body:
  successors: %bb.2(0x04000000), %bb.3(0x7c000000)

  %2:gr64 = PHI %8, %bb.1, %7, %bb.3
  %3:gr64 = PHI %0, %bb.1, %6, %bb.3
  %4:gr32 = PHI %11, %bb.1, %5, %bb.3
  %5:gr32 = ADD32rm %4, %2, 1, $noreg, 0, $noreg, implicit-def dead $eflags, debug-location !33 :: (load 4 from %ir.lsr.iv1, !tbaa !29)
  DBG_VALUE debug-use %5, debug-use $noreg, !14, !DIExpression(), debug-location !19
  DBG_VALUE debug-use %5, debug-use $noreg, !14, !DIExpression(), debug-location !19
  %6:gr64 = DEC64r %3, implicit-def $eflags, debug-location !21
  %7:gr64 = ADD64ri8 %2, 4, implicit-def dead $eflags, debug-location !21
  JE_1 %bb.2, implicit $eflags, debug-location !24
  JMP_1 %bb.3, debug-location !24

Why is this DBG_VALUE inserted twice?

This revision now requires changes to proceed.Mar 2 2018, 2:36 PM
vsk added a comment.Mar 2 2018, 2:51 PM

@aprantl looks to me like there's one dbg_value inserted per dbg.value instruction referring to %add. I asked for this to be filechecked explicitly.

@vsk just pointed out that the dbg.value was in the original input already. That's also a bug, but not in your patch :-)

aprantl accepted this revision.Mar 2 2018, 2:54 PM
This revision is now accepted and ready to land.Mar 2 2018, 2:54 PM
This revision was automatically updated to reflect the committed changes.