Page MenuHomePhabricator

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

Authored by dpaoliello on Sep 21 2020, 5:05 PM.

Details

Summary

Issue Details:
In order to support coroutine splitting, any multi-value PHI node in a coroutine is split into multiple blocks with single-value PHI Nodes, which then allows a subsequent transform to generate reload instructions as required (i.e., to reload the value if required if the coroutine has been resumed). This causes issues with EH pads (catchswitch and catchpad) as all pads within a catchswitch must have the same unwind destination, but the coroutine splitting logic may modify them to each have a unique unwind destination if there is a PHI node in the unwind cleanuppad that is set from values in the catchswitch and cleanuppad blocks.

Fix Details:
During splitting, if such a PHI node is detected, then create a "dispatcher" cleanuppad as well as the blocks with single-value PHI Nodes: thus the "dispatcher" is the unwind destination and it will detect which predecessor called it and then branch to the appropriate single-value PHI node block, which will then branch back to the original cleanuppad block.

Diff Detail

Event Timeline

dpaoliello created this revision.Sep 21 2020, 5:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2020, 5:05 PM
dpaoliello requested review of this revision.Sep 21 2020, 5:05 PM

Ran clang-format

Added all context lines

lxfind added inline comments.Sep 22 2020, 11:30 AM
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
205

Should this be

if (CatchSwitch->getUnwindDest() == BB)
  return true;

?
Or is it impossible to have multiple predecessors with CatchSwitch all pointing to BB somehow?

rnk added a comment.Sep 22 2020, 11:42 AM

Seems reasonable, if the new multi-value phi works with later passes.

llvm/lib/Transforms/Coroutines/CoroFrame.cpp
139

Isn't this still a multi-valued phi node? Is it OK because it has constant arguments?

GorNishanov accepted this revision.Sep 22 2020, 1:35 PM

LGTM.

llvm/lib/Transforms/Coroutines/CoroFrame.cpp
139

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.

This revision is now accepted and ready to land.Sep 22 2020, 1:35 PM
dpaoliello marked 3 inline comments as done.

Addressed feedback

dpaoliello added inline comments.Sep 22 2020, 1:56 PM
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
139

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).

205

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.

lxfind added inline comments.Sep 22 2020, 1:59 PM
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
114

Could we add an assertion if BB is guaranteed to be the unwind dest?

Add requested assert

dpaoliello marked an inline comment as done.Sep 22 2020, 2:08 PM
lxfind accepted this revision.Sep 22 2020, 2:08 PM
GorNishanov accepted this revision.Sep 22 2020, 4:18 PM

I believe that this commit has the required approvals: can someone please merge it?

@lxfind Do you have commit permissions? If so, could you please merge this in?

@lxfind Do you have commit permissions? If so, could you please merge this in?

Sure! Could you please rebase? Somehow I couldn't apply this patch locally.

Rebase to master

@lxfind Do you have commit permissions? If so, could you please merge this in?

Sure! Could you please rebase? Somehow I couldn't apply this patch locally.

Done - thanks!

@lxfind Do you have commit permissions? If so, could you please merge this in?

hmm I still cannot patch this from master. Can you double check that you don't have a local commit in the tree?

Regenerate diff

@lxfind Do you have commit permissions? If so, could you please merge this in?

hmm I still cannot patch this from master. Can you double check that you don't have a local commit in the tree?

Looks like it's an issue with using PowerShell on Windows to redirect the output of git diff to a file.
I regenerated with git built-in --output option, and it seems to be working - please try again with the latest patch.

lxfind added inline comments.Tue, Nov 10, 2:22 PM
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
1235

CS gets unused in release build, leading to a warning. You will need (void) it.