Skip to content
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

Update hash of the new CSE when resizing #61984

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Nov 23, 2021

The CSE logic maintains a custom hashtable implementation.

It triggers a resize at the same time as adding a new CSE, but forgets the hash of the new CSE needs to be updated from its pre-resize value. Failing to do so can lead to losing some CSEs, though not in a correctness-impacting way, I believe.

This was causing spurious diffs over at #61933.

We are expecting small diffs due to some new CSEs.

For example, here is one Win-x64 diff

We discover the CSE "earlier":

 CSE candidate #19, key=$1a51 in BB04, [cost= 4, size=12]:
-N008 (  4, 12) CSE #19 (use)[010348] #---G-------              *  IND       ref    $1a51
-N007 (  2, 10)              [010347] H-----------              \--*  CNS_INT(h) long   0xC46CA260 [ICON_STR_HDL] $14e2
+N008 (  4, 12) CSE #19 (use)[010304] #---G-------              *  IND       ref    $1a51
+N007 (  2, 10)              [010303] H-----------              \--*  CNS_INT(h) long   0xC46CA260 [ICON_STR_HDL] $14e2

 CSE candidate #20, key=$1ca2 in BB04, [cost= 4, size=12]:
 N008 (  4, 12) CSE #20 (use)[010700] #---G-------              *  IND       ref    $1ca2
@@ -332708,14 +332708,15 @@ BB04 [010216] Use of CSE #18 [weight=0.50]
 BB04 [004187] Use of CSE #01 [weight=0.50]
 BB04 [004198] Use of CSE #01 [weight=0.50]
 BB04 [004204] Use of CSE #01 [weight=0.50]
+BB04 [010260] Def of CSE #19 [weight=0.50]
 BB04 [004243] Use of CSE #01 [weight=0.50]
 BB04 [004254] Use of CSE #01 [weight=0.50]
 BB04 [004260] Use of CSE #01 [weight=0.50]
-BB04 [010304] Def of CSE #19 [weight=0.50]
+BB04 [010304] Use of CSE #19 [weight=0.50] *** Now Live Across Call ***
 BB04 [004299] Use of CSE #01 [weight=0.50]
 BB04 [004310] Use of CSE #01 [weight=0.50]
 BB04 [004316] Use of CSE #01 [weight=0.50]
-BB04 [010348] Use of CSE #19 [weight=0.50] *** Now Live Across Call ***
+BB04 [010348] Use of CSE #19 [weight=0.50]
 BB04 [004355] Use of CSE #01 [weight=0.50]
 BB04 [004366] Use of CSE #01 [weight=0.50]
 BB04 [004372] Use of CSE #01 [weight=0.50]

Full diffs.

The CSE logic maintains a custom hashtable implementation.

It triggers a resize at the same time as adding a new CSE,
but forgets the hash of the new CSE needs to be updated from
its pre-resize value. Failing to do so can lead to losing some
CSEs, though not in a correctness-impacting way.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 23, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 23, 2021
@ghost
Copy link

ghost commented Nov 23, 2021

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

The CSE logic maintains a custom hashtable implementation.

It triggers a resize at the same time as adding a new CSE, but forgets the hash of the new CSE needs to be updated from its pre-resize value. Failing to do so can lead to losing some CSEs, though not in a correctness-impacting way, I believe.

This was causing spurious diffs over at #61933.

We are expecting small diffs due to some new CSEs.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review November 24, 2021 09:00
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants