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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Seems reasonable. Adding a couple of folks who know a bit more about debug info than I do to approve this.
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 | ||
30 | 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? |
lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp | ||
---|---|---|
797 | Don't put braces around a single-line body. 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.
test/CodeGen/Generic/linear-dbg-value.ll | ||
---|---|---|
46 | please remove all non-essential attributes. (everything in quotes) |
test/CodeGen/Generic/linear-dbg-value.ll | ||
---|---|---|
30 | Thanks! Could you also check that the second dbg.value() for %add is inserted right after the first one? |
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.
test/CodeGen/Generic/linear-dbg-value.ll | ||
---|---|---|
29 | 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 :-) |
How does this patch get checked in? I don't have commit privileges, so far as I know.
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?
@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 :-)
Nit: could you used a range-for loop here and run clang-format over the diff?