This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Emit a BasePointer when we have overly-aligned stack objects
ClosedPublic

Authored by jgravelle-google on Nov 2 2016, 2:37 PM.

Details

Summary

Because we shift the stack pointer by an unknown amount, we need an
additional pointer. In the case where we have variable-size objects
as well, we can't reuse the frame pointer, thus three pointers.

Event Timeline

jgravelle-google retitled this revision from to [WebAssembly] Emit a BasePointer when we have overly-aligned stack objects.
jgravelle-google updated this object.
jgravelle-google added reviewers: dschuff, sunfish.
jgravelle-google added a subscriber: llvm-commits.
  • Move HasBP closer to its use
dschuff added inline comments.Nov 2 2016, 5:45 PM
lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
40

Maybe clarify that "overaligned" means more aligned than the stack pointer (rather than more than natural alignment for the slot itself).

test/CodeGen/WebAssembly/alignment.ll
1 ↗(On Diff #76791)

alignment.ll seems a bit generic. It could probably be stack-aligment or else these tests could just be added to userstack.ll

29 ↗(On Diff #76791)

Might as well check for the value of the const too, so we know the alignment is 16.

34 ↗(On Diff #76791)

Seems like we should be storing directly from BP. Where does this copy_local come from?
The epilog codegen looks ok; I guess this copy is the one generated in the prolog and gets moved down by the stackifier?

test/CodeGen/WebAssembly/alignment.ll
34 ↗(On Diff #76791)

Huh. This is caused by us emitting a frame pointer, and register stackification doing magic to it.
tl;dr: Yes.

Prolog/epilogue:

  1. load __stack_pointer into SP
  2. copy SP into BP
  3. decrement SP by fixed amount
  4. store new SP as FP
  5. [rest of code]
  6. store BP into __stack_pointer

Reg stackify:

  1. Moves the single copy into BP to be next to its use
  2. Tees the two uses of the original load of __stack_pointer, stackifies one for use in computing FP, and saves one in BP

The good news is this just gets translated to get_local in s2wasm. And adding -wasm-explicit-locals models it that was as well, so there's no actual cost here, it's just kinda quirky.
... is what I've been able to piece together.

  • Simplify SPReg, clarify comment, rename test, check stack size in test
dschuff accepted this revision.Nov 7 2016, 11:39 AM
dschuff edited edge metadata.
This revision is now accepted and ready to land.Nov 7 2016, 11:39 AM
This revision was automatically updated to reflect the committed changes.