[PATCH] D15344: [WebAssembly] WIP for SP lowering.

[Dan Gohman via llvm-commits]

sunfish added a comment.

I'm still reviewing this, but here are a few initial comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp:74
@@ +73,3 @@
+  if (!StackSize)
+    return;
+  const auto *TII = MF.getSubtarget().getInstrInfo();
----------------
We also need a prologue when MFI->adjustsStack(). Can you assert that doesn't happen for now?

================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp:84
@@ +83,3 @@
+  auto *SPSymbol = MF.getFunction()->getParent()
+      ->getNamedGlobal("SP");
+  BuildMI(MBB, InsertPt, DL, TII->get(WebAssembly::CONST_I32), SPReg)
----------------
Instead of using getNamedGlobal on the Module, which requires a declaration in LLVM IR, we can use MF.createExternalSymbolName (and addExternalSymbol below), which creates an ExternalSymbol, which is a very lightweight thing that doesn't require any up-front setup.

Also, "SP" is in the C/C++ user namespace. What do you think about "__stack_pointer"?

================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp:110
@@ -69,2 +109,3 @@
+      .addMemOperand(MMO);
 }
 
----------------
Can you assert MFI->getCalleeSavedInfo().empty(), just to double-check ourselves?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp:145
@@ -74,1 +144,3 @@
+      .addMemOperand(MMO);
+
 }
----------------
This code doesn't yet set up FP; can you assert !hasFP(MF) for now?

Can you add TODO comments for:
 - Do ".setMIFlag(MachineInstr::FrameSetup)" on emitted instructions
 - Emit TargetOpcode::CFI_INSTRUCTION instructions



http://reviews.llvm.org/D15344