Skip to content

C#: Refactor Structural Comparison for Control Flow Elements. #8038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Mar 9, 2022

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Feb 15, 2022

In this PR we re-factor the existing implementation of the structural comparison for controls flow elements.
The new approach is based on a global value number approach.

The idea is strongly linked to the global value numbering used in the type unification.
Please note that the implementation is still hidden under the existing interface, but we plan to make a follow up PR where this is removed.

@github-actions github-actions bot added the C# label Feb 15, 2022
@michaelnebel michaelnebel marked this pull request as ready for review February 15, 2022 15:09
@michaelnebel michaelnebel requested a review from a team as a code owner February 15, 2022 15:09
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Feb 15, 2022
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.

Added a couple of comments. This is a low confidence first review.

BTW, what's the benefit of doing the comparison with GVN vs the previous structural comparison?

@michaelnebel
Copy link
Contributor Author

Added a couple of comments. This is a low confidence first review.

BTW, what's the benefit of doing the comparison with GVN vs the previous structural comparison?

Thank you for looking into this!
The benefit should be better performance (and maybe also simpler code). Unfortunately, the DCA run shows a performance regression. I suspect that that the cause of this is due to some missing pragmas (some noinline and nomagic).

@michaelnebel michaelnebel force-pushed the csharp/gvn-cfecomparison branch 2 times, most recently from 395d10f to ec91e92 Compare February 25, 2022 09:13
@michaelnebel michaelnebel force-pushed the csharp/gvn-cfecomparison branch from ec91e92 to 23fbfbc Compare March 2, 2022 12:49
@michaelnebel
Copy link
Contributor Author

@hvitved: The DCA run now completed and it looks like there are no overall performance regressions. Some queries themselves might be a bit slower now, but maybe that could be linked to building the cache? There was identified a single "join order badness", but that is not side effect of this change.

@michaelnebel michaelnebel requested a review from hvitved March 3, 2022 08:23
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Looks very good to me! I have made some (mostly trivial) suggestions here.

@michaelnebel michaelnebel merged commit fbe8f75 into github:main Mar 9, 2022
@michaelnebel michaelnebel deleted the csharp/gvn-cfecomparison branch March 9, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants