Skip to content

Fix 97272#99818

Merged
EgorBo merged 1 commit into
dotnet:mainfrom
EgorBo:fix-recursive-intrinsics
Jun 15, 2024
Merged

Fix 97272#99818
EgorBo merged 1 commit into
dotnet:mainfrom
EgorBo:fix-recursive-intrinsics

Conversation

@EgorBo

@EgorBo EgorBo commented Mar 15, 2024

Copy link
Copy Markdown
Member

Fixes #97272

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 15, 2024
@EgorBo

EgorBo commented Mar 15, 2024

Copy link
Copy Markdown
Member Author

@MihuBot

@EgorBo

EgorBo commented Mar 15, 2024

Copy link
Copy Markdown
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines

Copy link
Copy Markdown
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
Copy Markdown
Contributor

MrBeanWaitingGIF

@Sergio0694

Copy link
Copy Markdown
Contributor

Me checking this PR before going to bed:

SchooldaysGIF

@EgorBo

EgorBo commented Apr 18, 2024

Copy link
Copy Markdown
Member Author

/azp list

@azure-pipelines

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@EgorBo

EgorBo commented Apr 18, 2024

Copy link
Copy Markdown
Member Author

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

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@Sergio0694

Copy link
Copy Markdown
Contributor

WaitingSpongebobGIF

@Sergio0694

Copy link
Copy Markdown
Contributor

CatLayingGIF

@EgorBo

EgorBo commented May 21, 2024

Copy link
Copy Markdown
Member Author

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

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@EgorBo

EgorBo commented May 22, 2024

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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).

Comment thread src/coreclr/jit/hwintrinsic.cpp Outdated
Comment thread src/coreclr/jit/importercalls.cpp Outdated
Comment thread src/coreclr/jit/importercalls.cpp
Comment thread src/coreclr/jit/hwintrinsic.cpp Outdated
@Sergio0694

Copy link
Copy Markdown
Contributor

IMissYouGIF

Egor pls

@EgorBo

EgorBo commented Jun 14, 2024

Copy link
Copy Markdown
Member Author

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

@tannergooding tannergooding left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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