[PATCH] D88059: [Coroutine] Split PHI Nodes in `cleanuppad` blocks in a way that obeys EH pad rules

[Daniel Paoliello via Phabricator via llvm-commits]

dpaoliello added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1145
+  // cleanuppad.corodispatch
+  //    %2 = phi i8[0, %catchswitch], [1, %catch.1]
+  //    %3 = cleanuppad within none []
----------------
GorNishanov wrote:
> rnk wrote:
> > Isn't this still a multi-valued phi node? Is it OK because it has constant arguments?
> The use of %2 is in the same basic block as the definition for the phi, so it cannot cross a suspend point and therefore we will not try to spill and reload it.
The subsequent phase that is transforming the single-valued phi nodes is trying to handle a value being written before a coro suspend and then read afterwards (hence the introduction of `reload` instructions). It is true that said subsequent transform will not handle this multi-valued phi node, *but* since the prior single-valued phi nodes are introduced by splitting an existing edge we are guaranteed to not have a coro suspend between the single-valued phi nodes and this multi-valued phi node, thus reading these values is safe (and does not require a `reload`).


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1211
+    if (auto *CatchSwitch = dyn_cast<CatchSwitchInst>(Pred->getTerminator())) {
+      return CatchSwitch->getUnwindDest() == BB;
+    }
----------------
lxfind wrote:
> Should this be
> ```
> if (CatchSwitch->getUnwindDest() == BB)
>   return true;
> ```
> ?
> Or is it impossible to have multiple predecessors with `CatchSwitch` all pointing to BB somehow?
That's a fair point: I think having this in its own function creates some ambiguity, since what I'm specifically looking for is a `cleanuppad` with a `catchswitch` predecessor (therefore the `cleanuppad` is an unwind destination). I'm going this check inline to make it clearer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88059/new/

https://reviews.llvm.org/D88059