Page MenuHomePhabricator

[WebAssembly] Fix stack pointer store check in RegStackify
ClosedPublic

Authored by aheejin on Dec 26 2018, 2:47 PM.

Details

Summary

We now use __stack_pointer global and global.set/global.get instruction.
This fixes the checking routine for stack_pointer writes accordingly.

This also fixes the existing __stack_pointer test in reg-stackify.ll:
That test used to pass not because of __stack_pointer clashes but
because the function stackpointer_callee was not marked as readnone,
so it was assumed to possibly write to memory arbitraily, and
global.set instruction was marked as mayStore in the .td definition,
so they were identified as intervening writes. After we added readnone
to its attribute, this test fails without this patch.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Dec 26 2018, 2:47 PM
aheejin marked an inline comment as done.Dec 26 2018, 2:49 PM
aheejin added inline comments.
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
196 ↗(On Diff #179529)

It looks like this part was added in D34172 and we were using wasm globals for __stack_pointer even by then, so I'm not 100% sure what this part meant, but anyway without this patch the modified version of stackpointer_callee in reg-stackify.ll fails.

aheejin marked an inline comment as done.Dec 26 2018, 2:50 PM
aheejin added inline comments.
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
196 ↗(On Diff #179529)

Oh not added in D34172.. modified maybe

aheejin edited the summary of this revision. (Show Details)Dec 26 2018, 2:53 PM
dschuff accepted this revision.Jan 7 2019, 10:28 AM

This patch looks fine with the comment nit. I'm a little unclear because get_global and set_global are still marked as mayLoad and mayStore. Is it a problem with both the explicit modeling of the stack pointer and treating them as reads and writes for this purpose?

lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
196 ↗(On Diff #179529)

We supported both C global and wasm global for a while (wasm global for object files and C global for s2wasm). This code is definitely for the latter and can be deleted. (BTW have we removed all support for C global stack pointer yet? I can't remember off the top of my head).

248 ↗(On Diff #179529)

We should call this a "write" or "set" and avoid using the term "store" for clarity.

This revision is now accepted and ready to land.Jan 7 2019, 10:28 AM
aheejin updated this revision to Diff 180598.Jan 7 2019, 6:04 PM
aheejin marked an inline comment as done.
  • stores -> writes
aheejin added a comment.EditedJan 7 2019, 6:12 PM

@sunfish By the way, when we check interfering read/write/sideeffect/stackpointer here to figure out if the given instruction is safe to move, all calls are assumed to touch the stack pointer. But aren't all calls supposed to restore stack pointer before they return? What is the reason we can't move a call over another call because of interfering stack pointer?

aheejin edited the summary of this revision. (Show Details)Jan 7 2019, 11:58 PM

@sunfish Ping on my previous question :)

@aheejin The issue is that the current code doesn't distinguish between reading and writing of the stack pointer. Calls don't effectively modify the stack pointer, but they do read the stack pointer. Instructions which modify the stack pointer shouldn't be reordered past calls.

So if we generalized this to track stack pointer reads separately from writes, then we could allow stack-pointer-reading instructions to be reordered across each other.

@aheejin The issue is that the current code doesn't distinguish between reading and writing of the stack pointer. Calls don't effectively modify the stack pointer, but they do read the stack pointer. Instructions which modify the stack pointer shouldn't be reordered past calls.

So if we generalized this to track stack pointer reads separately from writes, then we could allow stack-pointer-reading instructions to be reordered across each other.

I see, thanks!

aheejin updated this revision to Diff 181170.Jan 10 2019, 3:12 PM
  • SET_GLOBAL_I32 -> GLOBAL_SET_I32
This revision was automatically updated to reflect the committed changes.

Could this CL be the cause of the newly-passing test in https://ci.chromium.org/p/wasm/builders/luci.wasm.ci/linux/2560 ?

Oh, I'll take a look. Thank you.