Page MenuHomePhabricator

[DAG] Don't increase SDNodeOrder for dbg.value/declare.
ClosedPublic

Authored by uabelho on Oct 6 2016, 5:55 AM.

Details

Summary

The SDNodeOrder is saved in the IROrder field in the SDNode, and this
field may affects scheduling. Thus, letting dbg.value/declare increase
the order numbers may in turn affect scheduling.

Because of this change we also need to update the code deciding when
dbg values should be output, in ScheduleDAGSDNodes.cpp/ProcessSDDbgValues.

Dbg values now have the same order as the SDNode they are connected to,
not the following orders.

Test cases provided by Florian Hahn.

Diff Detail

Repository
rL LLVM

Event Timeline

uabelho updated this revision to Diff 73776.Oct 6 2016, 5:55 AM
uabelho retitled this revision from to [DAG] Don't increase SDNodeOrder for dbg.value/declare..
uabelho updated this object.
uabelho added a reviewer: bogner.
uabelho added a subscriber: llvm-commits.

Ping. Anyone has an opinion about this?

I suppose everyone agrees that dbg.values shouldn't affect code generation?

Ping. Anyone has an opinion about this?

I suppose everyone agrees that dbg.values shouldn't affect code generation?

Unquestionably, debug info should have no effect on code generation.
The question is who feels comfortable enough with SelectionDAG to have an informed opinion about fiddling with this Order stuff. Not me, that's for sure.

Yes the presence or absence of DbgValues should not affect the generated (assembly) code.

Do you have any idea why numbering the DbgValues affects the schedule? At a first glance I only found the node order used in comparisons and sorting which shouldn't be affected whether you count the DbgValues or not.

Yes the presence or absence of DbgValues should not affect the generated (assembly) code.

Do you have any idea why numbering the DbgValues affects the schedule? At a first glance I only found the node order used in comparisons and sorting which shouldn't be affected whether you count the DbgValues or not.

What I saw when I debugged this in my out-of-tree target was that in ScheduleDAGRRList.cpp, the IROrder is used e.g via bu_ls_rr_sort::operator() (via BURRSort, via SPQ->getNodeOrdering), affecting the order in which the instructions were placed in the final schedule, which in turn affected register allocation.

Turning on -debug printouts when compiling the example in the patch you can see that the result of the scheduler is different when the dbg.value is present.

Could you provide a small C/C++ source that demonstrates the problem using an in-tree target? That is, something where the generated code is different with and without -g, but applying your patch makes the difference go away. It is disappointing but not really surprising that no existing tests are affected.

Could you provide a small C/C++ source that demonstrates the problem using an in-tree target? That is, something where the generated code is different with and without -g, but applying your patch makes the difference go away. It is disappointing but not really surprising that no existing tests are affected.

I can try, but to be honest I think it's demonstrated by the two functions in the ll-file already.
By only adding a dbg.value the resulting code gets different and the difference goes away when applying the patch.

Ideally the whole output of the two functions should be compared though and not just the first pushq.

fhahn added a subscriber: fhahn.Dec 1 2016, 8:09 AM

I've tested your change and it solves my test case in D27261 as well. This patch is slightly nicer then mine (D27261) imo.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
977 ↗(On Diff #73776)

I think you could use if (!isa<DbgInfoIntrinsic>(I)) here.

aprantl added inline comments.Dec 1 2016, 8:33 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
976 ↗(On Diff #73776)

and a . at the end of the comment ;-)

977 ↗(On Diff #73776)

This would be not just shorter but also more future-proof.

test/DebugInfo/Generic/selectiondag-order.ll
5 ↗(On Diff #73776)

Is there a good reason for this? Does it make the testcase more stable?
If we only check the first instruction, sh/could we simplify the IR?

Glad to see someone else also noticing this problem! :)

We've been running with this patch on our out-of-tree target for two months now without seeing
any problems with it.

I completely agree with the first two comments. I'm not sure what to do with the test case.
Any suggestions? Maybe just use the tests in https://reviews.llvm.org/D27261 instead?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
976 ↗(On Diff #73776)

Ok!

977 ↗(On Diff #73776)

Good point, I agree!

test/DebugInfo/Generic/selectiondag-order.ll
5 ↗(On Diff #73776)

I simply didn't know how to write the test in the best way. I would like to check that the generated instructions are completely identical in the two cases since they only differ in a dbg.value in the input.

Now it's been a while since I saw this problem but I think I tried to reduce more but couldn't if I stil lwanted to see the diff in the output. Someone else might of course be more succesful though.

uabelho updated this revision to Diff 80036.Dec 2 2016, 12:34 AM

Fixed the first two comments. Test case is still the same.

uabelho marked 2 inline comments as done.Dec 2 2016, 12:36 AM
fhahn added a comment.Dec 2 2016, 2:26 AM

I've uploaded two test cases in D27261, one with checks for AArch64 and one with checks for X86. I think it would be good to use those, as the test code in this review wasn't problementic on Aarch64. ideally we would have a general test that checks that all produced assembler instructions are equal, but I don't think there's a way to do that with FileCheck (and without pulling in other tools as dependency for the test).

I've uploaded two test cases in D27261, one with checks for AArch64 and one with checks for X86. I think it would be good to use those, as the test code in this review wasn't problementic on Aarch64.

Sure! Should I simply remove "my" test and add "your" tests in this patch or how do we do it?

ideally we would have a general test that checks that all produced assembler instructions are equal, but I don't think there's a way to do that with FileCheck (and without pulling in other tools as dependency for the test).

Yep! That's kind of how we do it for our out-of-tree target and that's how I found this bug. We compile random C-programs with and without -g and verify that the .text sections in the object files are identical but of course it then pulls in other tools to be able to do that. :/

fhahn added a comment.Dec 2 2016, 3:22 AM

I've uploaded two test cases in D27261, one with checks for AArch64 and one with checks for X86. I think it would be good to use those, as the test code in this review wasn't problementic on Aarch64.

Sure! Should I simply remove "my" test and add "your" tests in this patch or how do we do it?

Yes I think the easiest thing would be for you to just add my tests to your review.

ideally we would have a general test that checks that all produced assembler instructions are equal, but I don't think there's a way to do that with FileCheck (and without pulling in other tools as dependency for the test).

Yep! That's kind of how we do it for our out-of-tree target and that's how I found this bug. We compile random C-programs with and without -g and verify that the .text sections in the object files are identical but of course it then pulls in other tools to be able to do that. :/

There's a utility in the clang repo (clang/utils/check_cfc), but I don't think it's used in any tests and we should have CodGen tests for the patch in llvm anyways.

uabelho updated this revision to Diff 80050.Dec 2 2016, 3:57 AM
uabelho updated this object.

Used the test cases from https://reviews.llvm.org/D27261

fhahn added a comment.Dec 17 2016, 2:58 PM

Ping. Do you think the updated test cases are sufficient or would it help to provide a C example? I have one lying around somewhere, but the test case is quite ugly.

The patch only changes the Order value for debug intrinsics, so that the Order of debug intrinsics is equal to the previous non-debug instruction. The order of non-debug instructions is not affected by the change (as far as I can tell), because all non-debug instruction still have distinct Order values.

The order of debug instructions between two non-debug instructions could change though, because their order now relies on the order returned by DAG->GetDbgValues(N), but this shouldn't be a problem, right?

Ping. The patch still applies to recent master and fixes a bunch of codegen inconsistencies for the LLVM test-suite. My previous comment gives some details why I think this change should be save to add

atrick accepted this revision.Jan 15 2017, 10:24 AM
atrick edited edge metadata.

In principle this makes sense to me. Go ahead if there are no objections from @bogner or @aprantl.

This revision is now accepted and ready to land.Jan 15 2017, 10:24 AM

Thank you @atrick!

So, @bogner and @aprantl, no objections?

fhahn added a comment.Jan 17 2017, 2:37 PM

Awesome, thanks for taking time to review the patch! And thanks @uabelho for your patience :)

This revision was automatically updated to reflect the committed changes.