[PATCH] D56094: [WebAssembly] Fix stack pointer store check in RegStackify

[Derek Schuff via Phabricator via llvm-commits]

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