[PATCH] D56094: [WebAssembly] Fix stack pointer store check in RegStackify
Derek Schuff via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 7 10:28:51 PST 2019
dschuff accepted this revision.
dschuff added a comment.
This revision is now accepted and ready to land.
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?
================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:196
- }
- }
} else if (MI.hasOrderedMemoryRef()) {
----------------
aheejin wrote:
> aheejin wrote:
> > 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.
> Oh not added in D34172.. modified maybe
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).
================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:248
+ // Check for stores to __stack_pointer global.
+ if (MI.getOpcode() == WebAssembly::SET_GLOBAL_I32 &&
----------------
We should call this a "write" or "set" and avoid using the term "store" for clarity.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56094/new/
https://reviews.llvm.org/D56094
More information about the llvm-commits
mailing list