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

Spill tree temp large vectors around calls #22311

Merged
merged 2 commits into from
Feb 2, 2019
Merged

Conversation

CarolEidt
Copy link

The code was there to handle lclVars, but there was an implicit assumption that non-lclVar large vector tree temps would never be live across a call.

Fixes the GitHub_20657 failure in #22253

@CarolEidt
Copy link
Author

@BruceForstall - this only fixes the GitHub_20657 case. The HFA tests in #22253 still fail - either reliably or sporadically.

@@ -1840,8 +1840,7 @@ class RefPosition
// A RefPosition refers to either an Interval or a RegRecord. 'referent' points to one
// of these types. If it refers to a RegRecord, then 'isPhysRegRef' is true. If it
// refers to an Interval, then 'isPhysRegRef' is false.
//
// Q: can 'referent' be NULL?
// referent can never be null.
Copy link
Author

Choose a reason for hiding this comment

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

This is unrelated but I happened upon this, and by construction referent is never null.

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

Looks ok to me, though I'm not a great person to review LSRA changes

The code was there to handle lclVars, but there was an implicit assumption that non-lclVar large vector tree temps would never be live across a call.

Fixes the GitHub_20657 failure in #22253
@BruceForstall
Copy link
Member

@CarolEidt Do you expect to merge this soon? Are you doing more testing or looking for more reviews?

@CarolEidt
Copy link
Author

@BruceForstall - I plan to merge this as soon as I get a clean test run. I just removed an overly aggressive assert; hopefully this test run will be clean.

@CarolEidt
Copy link
Author

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress2_jitstressregs1 Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked jitstress2_jitstressregs1 Build and Test

1 similar comment
@CarolEidt
Copy link
Author

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress2_jitstressregs1 Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked jitstress2_jitstressregs1 Build and Test

@CarolEidt
Copy link
Author

JIT\HardwareIntrinsics\X86\Avx2\Avx2_ro\Avx2_ro.exe timed out on the stress run, but passes locally. Retrying:
@dotnet-bot test Windows_NT arm64 Cross Checked jitstress2_jitstressregs1 Build and Test

@CarolEidt
Copy link
Author

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test

@CarolEidt
Copy link
Author

test OSX10.12 x64 Checked Innerloop Build and Test

@CarolEidt
Copy link
Author

This time JIT\superpmi\superpmicollect\superpmicollect.cmd Timed Out. I'm going to go ahead and merge this, since that test passed in the last go-round.
@dotnet/jit-contrib - I know there have been some timeout-related changes recently, but I don't know if we've considered increasing the timeouts for the stress jobs.

@CarolEidt CarolEidt merged commit 070b5d7 into dotnet:master Feb 2, 2019
@CarolEidt CarolEidt deleted the Fix22253 branch February 2, 2019 22:33
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Spill tree temp large vectors around calls

The code was there to handle lclVars, but there was an implicit assumption that non-lclVar large vector tree temps would never be live across a call.

Fixes the GitHub_20657 failure in dotnet/coreclr#22253

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

Successfully merging this pull request may close these issues.

2 participants