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.

Sign up for GitHub

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

Jump to bottom

Fix lowering usage of an unset LSRA field. #54731

Merged
merged 5 commits into from
Jun 28, 2021
Merged

Fix lowering usage of an unset LSRA field. #54731

merged 5 commits into from
Jun 28, 2021

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Jun 25, 2021 •
edited
Loading

The old code was using m_lsra->isRegCandidate(varDsc) that reads lvIsTracked field that is not set during lowering and until liveness. So we were reading default lvIsTracked=false and always saying that it is not a reg candidate.

The issue started to happen after we added HW support in VN/CSE, it started to produce wrong results, then we added more checks in codegen and it started to fail with assert.

No diffs, the codegen is now clever enough to skip bitcast node when the source is in memory. In theory, we still don't want to create it for TP purposes so we need something like "varIsAlreadyKnownToBeInMemory" but I will try to add it in another change.

Fixes #54466

@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 Jun 25, 2021
@sandreenko
Copy link
Contributor Author

PTAL @kunalspathak @dotnet/jit-contrib

BruceForstall
BruceForstall approved these changes Jun 25, 2021
View reviewed changes
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.

Any diffs?

@sandreenko
Copy link
Contributor Author

Any diffs?

It is good that you asked, I wrote that there is none but now I checked again and saw 1 that I lost in the thousands of SPMI downloading lines.

asm.coreclr_tests.pmi.Linux.x64.checked
Top method regressions (bytes):
          10 ( 0.27% of base) : 239371.dasm - BringUpTest:Main():int
1 total methods with Code Size differences (0 improved, 1 regressed), 0 unchanged.

added a comment to fix it later.

kunalspathak
kunalspathak approved these changes Jun 28, 2021
View reviewed changes
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM...are the CI failures related to your change?

@sandreenko
Copy link
Contributor Author

LGTM...are the CI failures related to your change?

part of them are, I am working on a fix, looks like the fix will have to be bigger than I thought.

@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr jitstress, runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@sandreenko sandreenko mentioned this pull request Jun 28, 2021
Closed
@sandreenko
Copy link
Contributor Author

sandreenko commented Jun 28, 2021 •
edited
Loading

The failures are unrelated (at least I have not seen any JIT asserts and they failed in the previous run, so looking like infa).
The regressions are small:
the worst is

coreclr_tests.pmi.Linux.arm.checked.mch 
Total bytes of delta: 110, 
9 total methods with Code Size differences (0 improved, 9 regressed)).

I will fix them in a separate PR (#54841), this is just correctness.

@sandreenko sandreenko merged commit 94f3355 into dotnet:main Jun 28, 2021
@sandreenko sandreenko deleted the GitHub_54466 branch June 28, 2021 22:33
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 29, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_componentize_de…
06e80d9 
…bugger2

* origin/main: (78 commits)
  Fix unreached during dump. (dotnet#54861)
  Fix lowering usage of an unset LSRA field. (dotnet#54731)
  Fix setting breakpoints on AVX 256 instructions and other 32 byte immediate instructions (dotnet#54786)
  Add perf_slow yaml (dotnet#54853)
  Faster type load for scenarios made more common by generic math (dotnet#54588)
  Make sure we consider buffer length when marshalling back Unicode ByValTStr fields (dotnet#54695)
  Add YieldProcessor implementation for arm (dotnet#54829)
  Remove ActiveIssue for dotnet#50968 (dotnet#54831)
  Enable System.Text.Json tests for Wasm AOT (dotnet#54833)
  Remove ActiveIssue for dotnet#51723 (dotnet#54830)
  Fix load exception on generic covariant return type (dotnet#54790)
  Obsolete X509Certificate2.PrivateKey and PublicKey.Key. (dotnet#54562)
  First round of converting System.Drawing.Common to COMWrappers (dotnet#54636)
  Fix alloc-dealloc mismatches (dotnet#54701)
  Add one-shot ECB methods
  [Mono] MSBuild Task housekeeping (dotnet#54485)
  Move iOS/tvOS simulator AOT imports in the Mono workload (dotnet#54821)
  Remove unnecessary char[] allocation from Uri.GetRelativeSerializationString (dotnet#54799)
  Reduce overhead of Enumerable.Chunk (dotnet#54782)
  Fix EnumMemberRefs always returning NULL (dotnet#54805)
  ...
@kunalspathak
Copy link
Member

Potential regression: DrewScoggins/performance-2#7131. Needs investigation.

@kunalspathak
Copy link
Member

Potential improvement: DrewScoggins/performance-2#7138

@kunalspathak
Copy link
Member

@sandreenko - do you mind taking a look at the regression?

@sandreenko
Copy link
Contributor Author

@sandreenko - do you mind taking a look at the regression?

I am planning to fix #54841 this week and it will most likely go away. I can't assign the issue to myself (it is in a fork).

@kunalspathak
Copy link
Member

Potential regression: DrewScoggins/performance-2#7131. Needs investigation.

@DrewScoggins - this is the one that needs a transfer.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SingleAccretion SingleAccretion SingleAccretion left review comments

@BruceForstall BruceForstall BruceForstall approved these changes

@kunalspathak kunalspathak kunalspathak approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vector2.One is not always <1, 1> in some case (silent bad codegen)
4 participants
@sandreenko @kunalspathak @BruceForstall @SingleAccretion