[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: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:
> aheejin wrote:
> > dschuff wrote:
> > > aheejin wrote:
> > > > 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?
> > > Stepping back, why can catch instructions get reordered? It seems odd that they should be if we model them as having side effects or control flow or whatever.
> > > 
> > > Stepping back, if we move `catch`es later, and insert `catch_all` later, is there a reason we even need to put the `catch` in the BB early? Why not just add them when we add `catch_all`? Is it just because we ISel it from catchpad? I guess if we could avoid reordering, then it wouldn't be so confusing.
> > > 
> > > >I'm not exactly sure about this. What prevents normal epilogues generated by WebAssemblyFrameLowering::emitEpilogue being moved after a call within the same BB?
> > > 
> > > I think actually what happens is that calls are modeled as modifying SP, so that is ordered wrt anything else that modifies SP. So maybe that should just work for us too
> > > Stepping back, why can catch instructions get reordered? It seems odd that they should be if we model them as having side effects or control flow or whatever.
> > It is currently marked as `hasCtrlDep`, but I still have noticed cases that `catch`es are reordered (mostly something like `i32.const` is hoisted before `catch`). I added `hasSideEffects` in D50919 just to be safer. But even with that I don't think we can be sure instructions like `i32.const` will not be reordered before `catch`, or something like this won't happen:
> > ```
> > ehpad:
> >   br bb2
> > 
> > bb3:
> >   catch
> > ```
> > 
> > So LateEHPrepare even considers cases like the latter and make sure `catch` will be the first instruction of an EH pad.
> > 
> > 
> > > Stepping back, if we move catches later, and insert catch_all later, is there a reason we even need to put the catch in the BB early? Why not just add them when we add catch_all? Is it just because we ISel it from catchpad? I guess if we could avoid reordering, then it wouldn't be so confusing.
> > The reason we select `catch` in the isel stage is because unlike `catch_all`, it produces a value. Instructions below `catch` use the value produced by the `catch`.
> > 
> > 
> > So LateEHPrepare even considers cases like the latter and make sure catch will be the first instruction of an EH pad.
> 
> Hm, interesting. 
> In the latter case, I guess we'd end up with control flow inside the catch block, which is OK even when there are instructions in the first BB. 
> 
> So getting back to this code, I guess  it's not actually necessary to move the insert point below catch, and it makes the explanation funny, but OTOH it probably results in more readable IR in the common case.
> I guess the comment could be something like:
> "Insert __stack_pointer restoring instructions at the beginning of each EH pad, after the catch instruction. (Catch instructions may have been reordered, and catch_all instructions have not been inserted yet, but those cases are handled in LateEHPrepare)."
> That's not much better than what you had, but it does state the simple case first, followed by the extra explanation, which IMO is a little simpler to read.
> 
Yes, it's not strictly necessary to move the insert point below catch. I did it because as you said it's gonna make code more readable in common cases. Fixed comments; I agree this is simpler and easier to understand. Thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D50980