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 failed assertion 'FPbased == FPbased2' #111787

Merged
merged 2 commits into from
Jan 30, 2025
Merged

Fix failed assertion 'FPbased == FPbased2' #111787

merged 2 commits into from
Jan 30, 2025

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Jan 24, 2025 •
edited
Loading

When lvaFrameAddress optimizes one of the temporary variables (referenced as FP-m) to be placed next to existing SP+n variable (eg. OutgoingArgSpace at SP+0) we may end up coalescing a pair-wise load/store between them. A previous attempt to handle the case only accounted for the lvaFrameAddress optimization being applied to both of the variables or neither of them, but it didn't correctly handle the mixed variant.

Fixes #111777

@filipnavara
Move assert to prevent incorrect trigger when lvaFrameAddress optimiz…
96e4e8b 
…ation merged SP+n and FP-m stores into a single pair-wise store
@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 Jan 24, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2025
@filipnavara
Copy link
Member Author

filipnavara commented Jan 24, 2025 •
edited
Loading

Can someone trigger runtime-coreclr libraries-jitstress pipeline on this PR please?

@filipnavara filipnavara changed the title [CI test] Fix assertion failed 'FPbased == FPbased2' [CI test] Fix failed assertion 'FPbased == FPbased2' Jan 24, 2025
@jakobbotsch
Copy link
Member

/azp run runtime-coreclr libraries-jitstress

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@filipnavara

This comment was marked as outdated.

Sign in to view
@filipnavara filipnavara reopened this Jan 24, 2025
@EgorBo
Copy link
Member

EgorBo commented Jan 24, 2025

/azp run runtime-coreclr libraries-jitstress

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@filipnavara filipnavara changed the title [CI test] Fix failed assertion 'FPbased == FPbased2' Fix failed assertion 'FPbased == FPbased2' Jan 24, 2025
@filipnavara filipnavara mentioned this pull request Jan 24, 2025
Closed
@filipnavara filipnavara marked this pull request as ready for review January 24, 2025 15:59
@filipnavara
Copy link
Member Author

Can we run /azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra to make sure that I didn't miss anything?

@danmoseley
Copy link
Member

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@filipnavara
Copy link
Member Author

The CrossApartmentQueryInterface_NoDeadlock test failure seems new and unrelated to this PR. The test was added 3 weeks ago and it likely only happens in the GC stress runs. /cc @jkoritzinsky

@jkoritzinsky
Copy link
Member

That's a known issue. It's on my list of things to look at.

@filipnavara
Copy link
Member Author

That's a known issue. It's on my list of things to look at.

I didn't see any other failure in the Azure history which is why I asked. I'm happy to know it's on the radar.

@AndyAyersMS
Copy link
Member

@EgorBo are you the right reviewer for this?

@kunalspathak kunalspathak self-requested a review January 30, 2025 01:00
kunalspathak
kunalspathak approved these changes Jan 30, 2025
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

src/coreclr/jit/emitarm64.cpp
@@ -12311,7 +12311,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp)

// If there are 2 GC vars in this instrDesc, get the 2nd variable
// that should be tracked.
adr2 = emitComp->lvaFrameAddress(varNum2, &FPbased2, true);
adr2 = emitComp->lvaFrameAddress(varNum2, &FPbased2, FPbased);
Copy link
Member

Choose a reason for hiding this comment

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

I forgot to tell, but can you add a comment about suppressFPtoSPRewrite in the method docs of lvaFrameAddress? OK to do in separate PR.

@kunalspathak kunalspathak merged commit 7693b6d into dotnet:main Jan 30, 2025
146 of 150 checks passed
grendello added a commit to grendello/runtime that referenced this pull request Jan 30, 2025
@grendello
Merge branch 'main' into dev/grendel/android-clr-host-local
dbeb51b 
* main: (31 commits)
  More native AOT Pri-1 test tree bring up (dotnet#111994)
  Fix BigInteger outerloop test (dotnet#111841)
  JIT: Run 3-opt once across all regions (dotnet#111989)
  JIT: Check for profile consistency throughout JIT backend (dotnet#111684)
  [JIT] Add legacy extended EVEX encoding and EVEX.ND/NF feature to x64 emitter backend (dotnet#108796)
  [iOS][globalization] Fix IndexOf on empty strings on iOS to return -1 (dotnet#111898)
  System.Speech: Use intellisense xml from dotnet-api-docs (dotnet#111983)
  [mono][mini] Disable inlining if we encounter class initialization failure (dotnet#111754)
  [main] Update dependencies from dotnet/roslyn (dotnet#111946)
  Update dependencies from https://github.com/dotnet/arcade build 20250129.2 (dotnet#111996)
  Try changing the ICustomQueryInterface implementation to always return NotHandled instead of Failed to defer back to the ComWrappers impl. (dotnet#111978)
  Combined dependency update (dotnet#111852)
  Replace OPTIMIZE_FOR_SIZE with feature switch (dotnet#111743)
  Fix failed assertion 'FPbased == FPbased2' (dotnet#111787)
  Add remark to `ConditionalSelect` (dotnet#111945)
  JIT: fix try region cloning when try is nested in a handler (dotnet#111975)
  Use IRootFunctions in Tensor.StdDev (dotnet#110641)
  Remove zlib dependencies from Docker containers (dotnet#111939)
  Avoid `Unsafe.As` for `Memory<T>` and `ReadOnlyMemory<T>` conversion (dotnet#111023)
  Cleanup membarrier portability (dotnet#111943)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JIT: Assertion failed 'FPbased == FPbased2' during 'Emit code'
7 participants
@filipnavara @jakobbotsch @EgorBo @danmoseley @jkoritzinsky @AndyAyersMS @kunalspathak