[WebAssembly] Fix stack pointer store check in RegStackify

[aheejin]

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.

[aheejin]

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]

lib/Target/WebAssembly/WebAssemblyRegStackify.cpp

196 ↗

(On Diff #179529)

Oh not added in D34172.. modified maybe

[dschuff]

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.

[aheejin]

• stores -> writes

[aheejin]

@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]

rebase

[aheejin]

@sunfish Ping on my previous question :)

[sunfish]

@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]

In D56094#1353124, @sunfish wrote:

@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]

• SET_GLOBAL_I32 -> GLOBAL_SET_I32

[dschuff]

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

[aheejin]

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

llvm/trunk/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp

static void Query(const MachineInstr &MI, AliasAnalysis &AA, bool &Read,
 
   // Check for loads.
   if (MI.mayLoad() && !MI.isDereferenceableInvariantLoad(&AA))
     Read = true;
 
   // Check for stores.
   if (MI.mayStore()) {
     Write = true;
-
-    // Check for stores to __stack_pointer.
-    for (auto MMO : MI.memoperands()) {
-      const MachinePointerInfo &MPI = MMO->getPointerInfo();
-      if (MPI.V.is<const PseudoSourceValue *>()) {
-        auto PSV = MPI.V.get<const PseudoSourceValue *>();
-        if (const ExternalSymbolPseudoSourceValue *EPSV =
-                dyn_cast<ExternalSymbolPseudoSourceValue>(PSV))
-          if (StringRef(EPSV->getSymbol()) == "__stack_pointer") {
-            StackPointer = true;
-          }
-      }
-    }
   } else if (MI.hasOrderedMemoryRef()) {
     switch (MI.getOpcode()) {
     case WebAssembly::DIV_S_I32:
     case WebAssembly::DIV_S_I64:
     case WebAssembly::REM_S_I32:
     case WebAssembly::REM_S_I64:
     case WebAssembly::DIV_U_I32:
     case WebAssembly::DIV_U_I64:
case WebAssembly::I64_TRUNC_U_F64:
       // to move them because overflow and invalid are Undefined Behavior.
       break;
     default:
       Effects = true;
       break;
     }
   }
 
+  // Check for writes to __stack_pointer global.
+  if (MI.getOpcode() == WebAssembly::GLOBAL_SET_I32 &&
+      strcmp(MI.getOperand(0).getSymbolName(), "__stack_pointer") == 0)
+    StackPointer = true;
+
   // Analyze calls.
   if (MI.isCall()) {
     unsigned CalleeOpNo = WebAssembly::getCalleeOpNo(MI);
     QueryCallee(MI, CalleeOpNo, Read, Write, Effects, StackPointer);
   }
 }
 
 // Test whether Def is safe and profitable to rematerialize.

llvm/trunk/test/CodeGen/WebAssembly/reg-stackify.ll

 ; Don't stackify a call past a __stack_pointer store.
 
 ; CHECK-LABEL: stackpointer_dependency:
 ; CHECK:      call {{.+}}, stackpointer_callee@FUNCTION,
 ; CHECK-NEXT: global.set __stack_pointer@GLOBAL,
 ; NOREGS-LABEL: stackpointer_dependency:
 ; NOREGS:      call stackpointer_callee@FUNCTION
 ; NOREGS:      global.set __stack_pointer
-declare i32 @stackpointer_callee(i8* readnone, i8* readnone)
+declare i32 @stackpointer_callee(i8* readnone, i8* readnone) nounwind readnone
 declare i8* @llvm.frameaddress(i32)
 define i32 @stackpointer_dependency(i8* readnone) {
   %2 = tail call i8* @llvm.frameaddress(i32 0)
   %3 = tail call i32 @stackpointer_callee(i8* %0, i8* %2)
   ret i32 %3
 }
 
 ; Stackify a call_indirect with respect to its ordering