Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Nov 25, 2020

This PR changes the CFG for pattern expressions (including not expressions). Example:

public static bool M3(object c) =>
    c is not null ? c is 1 : c is 2;
CFG before: before
CFG after: after

https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/782/
https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/799/
https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/804/

@hvitved hvitved force-pushed the csharp/cfg/not-pattern branch 4 times, most recently from c729a06 to 0ac516d Compare November 27, 2020 20:18
@hvitved hvitved force-pushed the csharp/cfg/not-pattern branch 2 times, most recently from 965f1a5 to 4f69a37 Compare January 15, 2021 08:49
@hvitved hvitved force-pushed the csharp/cfg/not-pattern branch 2 times, most recently from 9fa3857 to ad002ef Compare January 19, 2021 12:48
@hvitved hvitved marked this pull request as ready for review January 19, 2021 12:49
@hvitved hvitved requested a review from a team as a code owner January 19, 2021 12:49
tamasvajk
tamasvajk previously approved these changes Jan 21, 2021
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

tamasvajk
tamasvajk previously approved these changes Jan 21, 2021
@hvitved hvitved force-pushed the csharp/cfg/not-pattern branch from 2b6d534 to 221aebc Compare January 25, 2021 13:02
@hvitved
Copy link
Contributor Author

hvitved commented Jan 25, 2021

Rebased and added two new commits.

tamasvajk
tamasvajk previously approved these changes Jan 25, 2021
@hvitved
Copy link
Contributor Author

hvitved commented Jan 26, 2021

Fixed a join-order regression caused by 355edcb, and started a new differences job.

Comment on lines -1375 to +1389
c =
any(NestedCompletion nc |
this.lastFinally(last, nc.getAnInnerCompatibleCompletion(), nc.getOuterCompletion(),
nc.getNestLevel())
)
exists(int nestLevel |
c =
any(NestedCompletion nc |
this.lastFinally(last, nc.getAnInnerCompatibleCompletion(), nc.getOuterCompletion(),
nestLevel) and
// unbind
nc.getNestLevel() >= nestLevel and
nc.getNestLevel() <= nestLevel
)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems overly complicated. Can you show me what perf issue was solved by this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, it is complicated. It solves a bad-join order that was also there before this PR, but which I spotted when fixing the other bad join-order:

                      1986     ~0%       {5} r84 = SCAN ControlFlowGraphImpl::Statements::TryStmtTree::lastFinally_dispred#fffff#prev_delta AS I OUTPUT I.<4>, I.<0> 'this', I.<1> 'last', I.<2> 'c', I.<3>
                      29295324 ~1%       {5} r85 = JOIN r84 WITH Completion::TNestedCompletion#ffff_23#join_rhs AS R ON FIRST 1 OUTPUT R.<1> 'c', r84.<4>, r84.<1> 'this', r84.<2> 'last', r84.<3>
                      10451    ~0%       {4} r86 = JOIN r85 WITH Completion::NestedCompletion::getOuterCompletion_dispred#ff AS R ON FIRST 2 OUTPUT r85.<0> 'c', r85.<4>, r85.<2> 'this', r85.<3> 'last'
                      499      ~3%       {3} r87 = JOIN r86 WITH Completion::NestedCompletion::getAnInnerCompatibleCompletion_dispred#ff AS R ON FIRST 2 OUTPUT r86.<3> 'last', r86.<0> 'c', r86.<2> 'this'

The rewrite avoids joining first on getNestLevel(), so it instead becomes

                      1986    ~0%       {5} r93 = SCAN ControlFlowGraphImpl::Statements::TryStmtTree::lastFinally_dispred#fffff#prev_delta AS I OUTPUT I.<3>, I.<0> 'this', I.<1> 'last', I.<2> 'c', I.<4>
                      28433   ~0%       {5} r94 = JOIN r93 WITH Completion::NestedCompletion::getOuterCompletion_dispred#ff_10#join_rhs AS R ON FIRST 1 OUTPUT R.<1> 'c', r93.<3>, r93.<1> 'this', r93.<2> 'last', r93.<4>
                      1497    ~7%       {4} r95 = JOIN r94 WITH Completion::NestedCompletion::getAnInnerCompatibleCompletion_dispred#ff AS R ON FIRST 2 OUTPUT r94.<0> 'c', r94.<2> 'this', r94.<3> 'last', r94.<4>
                      1497    ~0%       {7} r96 = JOIN r95 WITH Completion::TNestedCompletion#ffff_3012#join_rhs AS R ON FIRST 1 OUTPUT r95.<1> 'this', r95.<2> 'last', r95.<3>, r95.<0> 'c', R.<1>, R.<2>, R.<3>
                      1497    ~0%       {7} r97 = SELECT r96 ON r96.<6> >= r96.<2>
                      499     ~0%       {7} r98 = SELECT r97 ON r97.<6> <= r97.<2>
                      499     ~3%       {3} r99 = SCAN r98 OUTPUT r98.<1> 'last', r98.<3> 'c', r98.<0> 'this'

@hvitved hvitved merged commit 39977e9 into github:main Jan 27, 2021
@hvitved hvitved deleted the csharp/cfg/not-pattern branch January 27, 2021 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants