Page MenuHomePhabricator

Improve virtual register handling when computing debug information
ClosedPublic

Authored by ddcc on Jul 20 2016, 1:48 PM.

Details

Summary

Some backends, like WebAssembly, use virtual registers instead of physical registers. This crashes the DbgValueHistoryCalculator pass, which assumes that all registers are physical. Instead, skip virtual registers when iterating aliases, and assume that they are clobbered.

Diff Detail

Repository
rL LLVM

Event Timeline

ddcc updated this revision to Diff 64752.Jul 20 2016, 1:48 PM
ddcc retitled this revision from to Ignore virtual registers when computing debug information.
ddcc updated this object.
ddcc added a reviewer: dschuff.
ddcc added subscribers: sunfish, jfb.
ddcc removed a subscriber: dschuff.
ddcc added a subscriber: llvm-commits.
dschuff edited edge metadata.Jul 20 2016, 2:27 PM

I think this surely isn't the final solution for debug info on virtual targets like wasm (that will probably require some more design work in the medium term), but not crashing is certainly a good start.
So, I think this code is fine. But the first part of the commit description isn't quite right, because it doesn't actually ignore virtual registers; it does track the clobbers (it just knows they don't have aliases and doesn't try to put them into the set of changing registers).

ddcc retitled this revision from Ignore virtual registers when computing debug information to Improve virtual register handling when computing debug information.Jul 20 2016, 2:39 PM
ddcc updated this object.
ddcc edited edge metadata.

Right, I've fixed the title.

dschuff accepted this revision.Jul 20 2016, 2:47 PM
dschuff edited edge metadata.

LGTM for a start; we'll wait a bit and see if there are other opinions before landing.

This revision is now accepted and ready to land.Jul 20 2016, 2:47 PM
aprantl edited edge metadata.Jul 21 2016, 10:46 AM

Looks like this could really benefit from a (MIR?) testcase.

Yeah, it would have to be a MIR testcase; I wasn't sure if those were ready for prime time (and in particular we don't have any in the wasm backend). I can look into that; FWIW the test cases in D21808 do cover this (although they are obviously less well-targeted since they are traditional ll->asm codegen test cases.

OK I looked a little more wrt tests. The problem with MIR in this case is that DbgValueHistoryCalculator isn't a pass, it's just part of the AsmPrinter pass, which isn't really set up for MIR testing (e.g. for use with llc -run-pass etc). Maybe we should just add a regular wasm codegen test? It's not quite ideal since this is target-independent code, but it would cover this, and it could still be pretty simple.

What about a test that uses MIR as *input* (so isn't affected by future codegen changes) and then checks DWARF output?

ddcc updated this revision to Diff 66197.Jul 29 2016, 4:52 PM
ddcc edited edge metadata.

Add test

ddcc added a comment.Jul 29 2016, 4:56 PM

I added a regular codegen test. I tried the MIR route, but ran into multiple bugs, including broken branch probability parsing, and no support for target passes.

Ouch, I think https://llvm.org/bugs/show_bug.cgi?id=24261 is probably a showstopper for MIR tests in wasm. Basically all the interesting things we'd want to test are target passes, and some of those passes need to be run in order to do anything interesting at all. I'm guessing the problem for this particular test is that you'd need to start after WebAssemblyRegNumbering in order to feed this into the asmprinter?

Also, looking at the branch probability parsing code, it looks like https://llvm.org/bugs/show_bug.cgi?id=28751 has been broken since December D:

ddcc added a comment.Aug 1 2016, 11:28 AM

Yes, without running the target passes for WebAssembly, llc will just crash later due to the virtual registers. Is this patch OK to land as is with the regular test?

I think we should land this with a regular test, because that's better than no test. We should also fix the llc pass runner because I would really like to have some more targeted tests for the wasm backend passes, but I don't want to block you on that.

ddcc added a comment.Aug 8 2016, 5:45 PM

Is this OK to merge?

I'm planning to simplify DbgValueHistoryCalculator in the near future so I'll need all the test coverage I can get :-)

lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp
249 ↗(On Diff #66197)

I can comment out this condition and the test still passes.

ddcc updated this revision to Diff 67601.Aug 10 2016, 2:02 PM

Update test

ddcc marked an inline comment as done.Aug 10 2016, 2:03 PM
aprantl accepted this revision.Aug 11 2016, 10:33 AM
aprantl edited edge metadata.

Thanks!

This revision was automatically updated to reflect the committed changes.

Thanks for pushing this through to the end!