This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Track frame registers through VReg and local allocation
ClosedPublic

Authored by dschuff on Dec 18 2019, 4:56 PM.

Details

Summary

This change has 2 components:

Target-independent:
add a method getDwarfFrameBase to TargetFrameLowering. It describes how the Dwarf frame base will be encoded.
That can be a register (the default), the CFA (which replaces NVPTX-specific logic in DwarfCompileUnit), or a DW_OP_WASM_location descriptr.

WebAssembly:
Allow WebAssemblyFunctionInfo::getFrameRegister to return the correct virtual register instead of FP32/SP32 after WebAssemblyReplacePhysRegs has run.
Make WebAssemblyExplicitLocals store the local it allocates for the frame register. Use this local information to implement getDwarfFrameBase

The result is that the DW_AT_frame_base attribute is correctly encoded for each subprogram, and each param and local variable has a correct DW_AT_location that uses DW_OP_fbreg to refer to the frame base.

Diff Detail

Event Timeline

dschuff created this revision.Dec 18 2019, 4:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 18 2019, 4:56 PM
dschuff added a comment.EditedDec 18 2019, 5:07 PM

I think that this approach to tracking the frame register through VReg and local substitution will work fairly reliably for now, because the frame register is live through the whole function and is unlikely to be coalesced with other locals or stackified.
One thing I don't like about D69807 is that getFrameRegister is called in a variety of places during generation of various bits of the code as well as in DwarfCompileUnit, whereas getFrameBaseLocation only has this particular meaning and used here (while in the other places getFrameRegister is returning phsyical registers that are meaningless partway through the pipeline.

I had hoped that I could avoid introducing a cross-cutting abstraction like getFrameBaseLocation by having the DWARF code either check the target (as it does with NVPTX) or check whether getFrameRegister returns a physreg (as it does currently, in order to ignore vregs); but then we still don't have a way to actually get the local number to encode, because it's stored in a data structure that's in the WebAssembly backend and may not even be compiled in.
So I'm not sure if we can find a good way to get it from DwarfCompileUnit.

dschuff updated this revision to Diff 235004.Dec 20 2019, 11:39 PM
  • use getDwarfFrameBase on WebAssemblyFrameLowering
  • Don't optimize live intervale for frame reg. use global instead of local when not needsSP
  • Update NVPTX
dschuff updated this revision to Diff 236726.Jan 7 2020, 5:39 PM
  • update tests, add new test for dwarf local var
  • start update multi-return test
dschuff marked 2 inline comments as done.Jan 7 2020, 5:44 PM
dschuff added a subscriber: tlively.
dschuff added inline comments.
llvm/test/CodeGen/WebAssembly/multi-return.ll
0–1

@tlively I re-ran update_llc_test_checks.py over this file and it removed all the CHECKs entirely (best kind of correct...). Am I holding it wrong?

14–15

The reason all the tests in this file change is because I exempted the frame registers from OptimizeLiveIntervals.
An alternative to just updating it would be to do what I did for the first 2 tests and use symbolic variables for the registers. @tlively are these reasonable names? Are these exactly the correct CHECKs? (It's slightly non-obvious exactly what these are checking)

Unfortunately I do not know how multi-return.ll works. Despite its name, it is not a multivalue test (although once multivalue works it would make sense to add a multivalue-enabled run to that file). I generally don't use test-generation scripts because they tend to produce tests with unclear goals, as you observe.

dschuff updated this revision to Diff 236904.Jan 8 2020, 2:19 PM
  • finish update multi-return
  • remove debug prints
  • clean up, add comments
  • clang-format
dschuff retitled this revision from [WIP] [WebAssembly] Track frame registers through VReg and local allocation to [WebAssembly] Track frame registers through VReg and local allocation.Jan 8 2020, 2:57 PM
dschuff edited the summary of this revision. (Show Details)
dschuff added reviewers: yurydelendik, sunfish.
dschuff updated this revision to Diff 236916.Jan 8 2020, 2:58 PM
  • add doxygen comment
dschuff added a comment.EditedJan 8 2020, 3:07 PM

This is ready for review now, please take a look.

Note for the wasm-backend folks:
The original virtual register philosophy in the backend was to eliminate the physical registers early and treat all the registers the same, which made things simpler. This CL walks back on that strategy a little bit, treating the frame registers specially in a couple of places. Currently the changes are quite simple but it's possible that we may find more places where we have to take those registers into account to get good debug information. An alternative might be to go back to having physical registers, which I'm guessing would add some complexity in a few places (which would be different places than the ones this CL introduces); it's not obvious which would be better.

I think in the longer term though that we might just want to use Dwarf CFI instead, and use the CFA instead of an explicit register as the frame base.
Even if we do that though, I think the TargetFrameLowering change is a good one because it eliminates the NVPTX oddity (and if we go to CFI we'd be using the same code). And this should cover the large majority of debugging cases in the short term, so it's worth putting in for now.

dschuff updated this revision to Diff 237820.Jan 13 2020, 5:11 PM

Rebase

  • suppress build warning
dschuff updated this revision to Diff 237823.Jan 13 2020, 5:27 PM
  • Fix NVPTX
yurydelendik accepted this revision.Jan 15 2020, 12:58 PM

Agree with all the changes

This revision is now accepted and ready to land.Jan 15 2020, 12:58 PM

Thanks, I'll commit this soon.

dschuff updated this revision to Diff 238379.Jan 15 2020, 3:16 PM
  • Cleanup scope, update lld debuginfo test
This revision was automatically updated to reflect the committed changes.