Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix initialization of resolution sets #8840

Merged
merged 1 commit into from
Jan 12, 2017
Merged

Conversation

CarolEidt
Copy link

This was causing a failure during dumping.
Fixing it exposed another dumping failure in crossgen of System.Private.Corelib.dll on x86, due to a dead node. I've fixed the assert, but I believe the dead node should have been eliminated. I filed issue #8839 for this.

Fix #8824

@CarolEidt
Copy link
Author

@sivarv @dotnet/jit-contrib PTAL

@@ -1914,6 +1914,8 @@ bool LinearScan::isRegCandidate(LclVarDsc* varDsc)

void LinearScan::identifyCandidates()
{
VarSetOps::AssignNoCopy(compiler, resolutionCandidateVars, VarSetOps::MakeEmpty(compiler));
Copy link

Choose a reason for hiding this comment

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

Any reason this cannot be done in the class constructor or do you prefer having these here?

Copy link
Author

Choose a reason for hiding this comment

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

Any reason this cannot be done in the class constructor or do you prefer having these here?

Took me awhile to track this down but it can't be done in the constructor because at that point the set of tracked lclVars hasn't been finalized, so the epoch on the bit sets doesn't match. I've added a comment.

Copy link
Author

Choose a reason for hiding this comment

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

Any reason this cannot be done in the class constructor or do you prefer having these here?

Took me awhile to track this down but it can't be done in the constructor because at that point the set of tracked lclVars hasn't been finalized, so the epoch on the bit sets doesn't match. I've added a comment.

// i.e. in the "localDefUse" case.
// There used to be an assert here that we wouldn't spill such a node.
// However, we can have unused lclVars that wind up being the node at which
// it is spilled. This probably indicates a bug, but we don't realy want to
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Typo "realy" instead of "really"

@sivarv
Copy link
Member

sivarv commented Jan 7, 2017

Looks good.

@sivarv
Copy link
Member

sivarv commented Jan 7, 2017

Ubuntu x64 leg has a failure: GC.Scenarios.DoublinkList.doublinkgen.doublinkgen
I remember seeing failure of this test in the recent past too intermittently.
Not sure whether that failure is related to this PR or another instance of intermittent failure. If it is latter, please file a tracking issue.

@mikedn
Copy link

mikedn commented Jan 7, 2017

Not sure whether that failure is related to this PR or another instance of intermittent failure. If it is latter, please file a tracking issue.

Already there: #6574

This was causing a failure during dumping.
Fixing it exposed another dumping failure in crossgen of System.Private.Corelib.dll on x86, due to a dead node. I've fixed the assert, but I believe the dead node should have been eliminated. I filed issue #8839 for this.
@CarolEidt
Copy link
Author

Any reason this cannot be done in the class constructor or do you prefer having these here?

Took me awhile to track this down but it can't be done in the constructor because at that point the set of tracked lclVars hasn't been finalized, so the epoch on the bit sets doesn't match. I've added a comment.

@CarolEidt CarolEidt merged commit d69811c into dotnet:master Jan 12, 2017
@CarolEidt CarolEidt deleted the Fix8824 branch May 23, 2017 00:53
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Fix initialization of resolution sets

Commit migrated from dotnet/coreclr@d69811c
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RyuJIT/x86/Checked] ngen dump assert
5 participants