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

Fix 97272 #99818

Merged
merged 1 commit into from
Jun 15, 2024
Merged

Fix 97272 #99818

merged 1 commit into from
Jun 15, 2024

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Mar 15, 2024

Fixes #97272

@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 Mar 15, 2024
@EgorBo
Copy link
Member Author

EgorBo commented Mar 15, 2024

@MihuBot

@EgorBo
Copy link
Member Author

EgorBo commented Mar 15, 2024

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo EgorBo marked this pull request as ready for review March 15, 2024 18:11
@Sergio0694
Copy link
Contributor

MrBeanWaitingGIF

@Sergio0694
Copy link
Contributor

Me checking this PR before going to bed:

SchooldaysGIF

@EgorBo
Copy link
Member Author

EgorBo commented Apr 18, 2024

/azp list

Copy link

CI/CD Pipelines for this repository:

@EgorBo
Copy link
Member Author

EgorBo commented Apr 18, 2024

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Sergio0694
Copy link
Contributor

WaitingSpongebobGIF

@EgorBo
Copy link
Member Author

EgorBo commented May 21, 2024

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented May 22, 2024

@tannergooding or anyone from @dotnet/jit-contrib could you take a look?
tl;dr: we currently detect must-expand intrinsics as "does this call invoke current root method" while it should be - "does this call invoke itself" regardless of who is the root - this led to problems with [Intrinsic] attribute on top of virtual methods in Type type. The interesting side-effect from this is that we now can inline must-expand SIMD intrsinsics which we give up due to non-constant args for IMM etc. It allows jit to fold the fallback if late phases detect a constant. Example:

Vector128<int> Foo(Vector128<int> v)
{
    byte mask = 0b11110000;
    return Sse2.Shuffle(v, mask);
}

Main:

; Method MyBench:Foo
       mov      rcx, rdx
       mov      rdx, r8
       mov      r8d, 240
       tail.jmp [Sse2:Shuffle(Vector128`1[int],ubyte):Vector128`1[int]]
; Total bytes of code: 18

PR:

; Method MyBench:Foo
       vpshufd  xmm0, xmmword ptr [r8], -16
       vmovups  xmmword ptr [rdx], xmm0
       mov      rax, rdx
       ret      
; Total bytes of code: 14

Diffs

There a few regressions which are just PMI-artifacts, e.g. we inline the fallback here. but it shouldn't matter since the method is supposed to be inlined (AggressiveInlining).

  • a couple of r2r regressions - I investigated those and looks like they're "cancel inlining because BlockNonDeterministicIntrinsics said so"

@tannergooding
Copy link
Member

There's two libraries-pmi regressions that look interesting. I think in production they aren't actually hit for the cases highlighted because they are small methods that get inlined and the constant is propagated.

But, it showcases an issue where we aren't preserving it as a call for the non-constant case, so we end up expanding the jump table inline instead, this can lead to nasty code explosion in some cases and that is potentially concerning (particularly for T0 like code).

@Sergio0694
Copy link
Contributor

IMissYouGIF

Egor pls

@EgorBo
Copy link
Member Author

EgorBo commented Jun 14, 2024

@tannergooding PTAL again, your work to rewrite HWINTRINSIC as calls helped to remove the hacks I had in the previous version.

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

This looks a lot better now, thanks!

@Sergio0694
Copy link
Contributor

finished-elijah-wood

@github-actions github-actions bot locked and limited conversation to collaborators Jul 16, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion failed '!"Unhandled must expand intrinsic, throwing PlatformNotSupportedException"' in Checked mode
3 participants