This is an archive of the discontinued LLVM Phabricator instance.

[SplitEdge] Update SplitCriticalEdge to return a nullptr only when the edge is not critical
ClosedPublic

Authored by sidbav on Jan 13 2021, 11:02 AM.

Details

Summary

The function SplitCriticalEdge (called by SplitEdge) can return a nullptr in cases where the edge is a critical. SplitEdge uses SplitCriticalEdge assuming it can always split all critical edges, which is an incorrect assumption.

The three cases where the function SplitCriticalEdge will return a nullptr is:

  1. DestBB is an exception block
  2. Options.IgnoreUnreachableDests is set to true and isa(DestBB->getFirstNonPHIOrDbgOrLifetime()) is not equal to a nullptr
  3. LoopSimplify form must be preserved (Options.PreserveLoopSimplify is true) and it cannot be maintained for a loop due to indirect branches

For each of these situations they are handled in the following way:

  1. Modified the function ehAwareSplitEdge originally from llvm/lib/Transforms/Coroutines/CoroFrame.cpp to handle the cases when the DestBB is an exception block. This function is called directly in SplitEdge. SplitEdge does not call SplitCriticalEdge in this case
  2. Options.IgnoreUnreachableDests is set to false by default, so this situation does not apply.
  3. Return a nullptr in this situation since the SplitCriticalEdge also returned nullptr. Nothing we can do in this case.

Diff Detail

Event Timeline

sidbav created this revision.Jan 13 2021, 11:02 AM
sidbav requested review of this revision.Jan 13 2021, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2021, 11:02 AM
sidbav retitled this revision from [SplitEdge] Update to handle cases which were not previously considered when the edge is critical. to [SplitEdge] Update SplitCriticalEdge to return a nullptr only when the edge is not critical.Mar 3 2021, 9:21 AM

Ping

asbirlea accepted this revision.Mar 11 2021, 5:35 PM

I'd have liked the thoughts of folks familiar with coroutines here, but overall the change looks like an improvement.

This revision is now accepted and ready to land.Mar 11 2021, 5:35 PM
sidbav added a comment.Apr 6 2021, 2:17 PM

I am going to commit this change since no one else seems to be reviewing it... will welcome any post commit reviews.

Could use an IR test. Is only CoroFrame.cpp affected?
I'd recommend to split this up into NFC refactoring patch, and the actual functionality change.

This revision was landed with ongoing or failed builds.Apr 6 2021, 2:26 PM
This revision was automatically updated to reflect the committed changes.
sidbav added a comment.Apr 6 2021, 2:39 PM

Could use an IR test. Is only CoroFrame.cpp affected?

Decided to use a unit test for this patch since I modified the functionality of SplitCriticalEdge and SplitEdge, and these functions are tested with unit tests. Used a unit test to be consistent.
CoroFrame.cpp, and BasicBlockUtils and BreakCriticalEdges are modified as well.

Could use an IR test. Is only CoroFrame.cpp affected?

Decided to use a unit test for this patch since I modified the functionality of SplitCriticalEdge and SplitEdge, and these functions are tested with unit tests. Used a unit test to be consistent.
CoroFrame.cpp, and BasicBlockUtils and BreakCriticalEdges are modified as well.

This doesn't really answer my question, sorry.
Which transformation passes benefit from this change?
Can there not be a lit test for that for them?

Could use an IR test. Is only CoroFrame.cpp affected?

Decided to use a unit test for this patch since I modified the functionality of SplitCriticalEdge and SplitEdge, and these functions are tested with unit tests. Used a unit test to be consistent.
CoroFrame.cpp, and BasicBlockUtils and BreakCriticalEdges are modified as well.

This doesn't really answer my question, sorry.
Which transformation passes benefit from this change?
Can there not be a lit test for that for them?

Oh, I thought you were asking which files were changed!

CoroFrame.cpp is not really affected as this is more of a NFC for CoroFrame.
However, quite a few other transformations that make use of SplitCriticalEdge and/or SplitEdge. I can create a lit test for those transformations.