[PATCH] D50980: [WebAssembly] Restore __stack_pointer after catch instructions

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyEHRestoreStackPointer.cpp:58
+  if (MF.getTarget().getMCAsmInfo()->getExceptionHandlingType() !=
+          ExceptionHandling::Wasm ||
+      !MF.getFunction().hasPersonalityFn() || !MF.getFrameInfo().hasCalls())
----------------
jgravelle-google wrote:
> This is kinda awkwardly indented, was this clang-formatted?
> Might be worth extracting a temporary variable here for the `getExceptionHandlingType()` call, i.e. `auto EHType = MF.getTarget().getMCAsmInfo()->getExceptionHandlingType();`, which should make the line-breaks more consistent.
> 
> ... Reading further down this patch, can we just call `needsPrologForEH` here?
> This is kinda awkwardly indented, was this clang-formatted?
Yes it was.

> ... Reading further down this patch, can we just call needsPrologForEH here?
Good idea. Done.

> Might be worth extracting a temporary variable here for the getExceptionHandlingType() call, i.e. auto EHType = MF.getTarget().getMCAsmInfo()->getExceptionHandlingType();, which should make the line-breaks more consistent.
Done in `WebAssemblyFrameLowering::needsPrologForEH`.


================
Comment at: lib/Target/WebAssembly/WebAssemblyEHRestoreStackPointer.cpp:73
+    // instructions. We hoist them to the top of EH pads in LateEHPrepare, so
+    // here we just insert instructions after a catch if it stays at the top.
+    auto InsertPos = MBB.begin();
----------------
dschuff wrote:
> This comment is a little confusing. Looking at the code, I guess it means we just always insert the fixup at the top of the BB, (but after the catch if it hasn't been moved)?
> What prevents the SP restoration from being moved after a call in the catch block?
> This comment is a little confusing. Looking at the code, I guess it means we just always insert the fixup at the top of the BB, (but after the catch if it hasn't been moved)?
Yes that's what it tries to do. Do you have any suggestions on making it less confusing? What I tried to say is, at this point, we have `catch` by this point but not `catch_all` (it will be generated later in LateEHPrepare). And even if we have `catch`, it may not be staying at the top. But here we don't want to go out of the way to find the `catch` instruction and generate these instructions after it because it will be done in LateEHPrepare anyway.

> What prevents the SP restoration from being moved after a call in the catch block?
I'm not exactly sure about this. What prevents normal epilogues generated by `WebAssemblyFrameLowering::emitEpilogue` being moved after a call within the same BB?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFrameLowering.h:49
+  /// Write SP back to __stack_pointer global.
+  void writeSPToMemory(unsigned SrcReg, MachineFunction &MF,
+                       MachineBasicBlock &MBB,
----------------
dschuff wrote:
> Since SP is now a global and not a memory address anymore, we should change this function name. I realize that's pre-existing for this change, so it can be in a different CL (either before or after this one, whatever is convenient).
Done in D51046, which should land before this one.


Repository:
  rL LLVM

https://reviews.llvm.org/D50980