x86: support Return SIMD8. #46899

sandreenko · 2021-01-13T02:31:04Z

Support trees like:

N002 (  4,  3) [000055] ------------              *  RETURN    simd8  $304
N001 (  3,  2) [000054] -------N----              \--*  LCL_VAR   simd8 <System.Numerics.Vector2> V09 tmp3         u:1 (last use) $302

that we return in EAX/EDX.

Also, fix a few non-functional bugs like wrong printing or wrong instruction attributes that are not read.
Unblocks #46238

sandreenko · 2021-01-13T04:36:27Z

/azp list

azure-pipelines · 2021-01-13T04:36:32Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr clrinterpreter runtime-coreclr crossgen2-composite gcstress runtime-libraries-mono outerloop runtime-staging

sandreenko · 2021-01-13T04:36:46Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2021-01-13T04:37:05Z

Azure Pipelines successfully started running 1 pipeline(s).

sandreenko · 2021-01-13T08:18:58Z

as expected x86 does not use it currently and x64 uses only for simd16. The types were wrong for x64 but they were not used.

sandreenko · 2021-01-15T00:53:00Z

PTAL @dotnet/jit-contrib , cc @jkoritzinsky

jkoritzinsky

LGTM based on my knowledge. Just one comment to clarify.

jkoritzinsky · 2021-01-15T01:07:46Z

src/coreclr/jit/codegenxarch.cpp

+    // reg0 = opReg[31:0]
+    inst_RV_RV(ins_Copy(opReg, TYP_INT), reg0, opReg, TYP_INT);
+    // reg1 = opRef[63:32]
+    inst_RV_RV_IV(INS_pextrd, EA_4BYTE, reg1, opReg, 1);


Are we guaranteed to be able to use an SSE4.1 instruction here?

If not, we should be able to use PEXTRW twice along with a 16 bit shift to emulate it. (IIRC SSE2 is our minimum instruction set support for x86)

I can't find 5.0 requirements for some reason, @echesakovMSFT or @tannergooding probably know better.

In case SSE2 is our minimum, here's an instruction sequence that I think would be SSE2-compatible with equivalent results (given the return value is in xmm0) to save you time if needed:

pextrw eax, xmm0, 3 shl eax, 16 pextrw edx, xmm0, 2 or edx, eax movd eax, xmm0

Yes, I agree with @jkoritzinsky - you would need to check for compExactlyDependsOn(InstructionSet_SSE41) before emitting pextrd in a similar way as it is done here

runtime/src/coreclr/jit/hwintrinsicxarch.cpp

Line 1218 in 08c3065

if (!compExactlyDependsOn(InstructionSet_SSE41))

In addition, please make sure that we don't emit that instruction during crossgen.

I believe you want compOpportunisticallyDependsOn. The SSE2 vs SSE4.1 paths are equivalent, it's just that the SSE4.1 path is "faster" and should be preferred when it is available.

For crossgen1 SSE4.1 should be false, for crossgen2, it should depend on the ISA configuration (which I believe currently defaults to SSE4.1) and should flag the method as "uses SSE4.1" in which case it will be rejitted if false.

IIRC, there are some special considerations when it's all internal and therefore a IsSupported check doesn't exist, but @jkotas and @davidwrighton are the two most familiar with this (David wrote the original logic and Jan made some recent changes in the area).

I believe you want compOpportunisticallyDependsOn

+1

compOpportunisticallyDependsOn is what you want and it will do exactly the right thing when using crossgen2, but I believe it is not safe for this kind of purpose when compiling S.P.C with crossgen1. As we haven't quite gotten rid of crossgen1, you'll need to find some way to not generate the SSE4.1 support when crossgening S.P.C with original crossgen, or at least, wait until #47019 is in and enabled for both X86 and X64.

I would prefer to merge this PR as is with unsafe behavior for x86 old machines because:

this code is emitted only for PInvoke stubs that we don't crossgen, so no real issue here;

it could only affect old machines;

it will automatically be fixed with a full switch to crossgen2;

the alternative is to merge the change with SSE2 and open an issue to return SSE4.1 code after crossgen1 deprecation.

Good news. Crossgen2 was put in place to replace compilation for S.P.C over the weekend. The only time that S.P.C should now be compiled with crossgen1 is in self-contained app build scenarios before we transition those over as well, and as you state there is no actual use of this feature in S.P.C.

In light of that, I believe this is ok to check in.

src/coreclr/jit/instr.cpp

src/coreclr/jit/codegenxarch.cpp

sandreenko · 2021-01-15T20:28:15Z

Thanks, @tannergooding, the PR was updated based on your review.

This should probably go through inst_RV_TT_IV.

The jit does not currently have a clear contract when to call GetEmitter()->emitIns vs CodeGen-> inst_* and it is often confusing. I did not want to useinst_RV_TT_IV there because it takes a tree node when we already know the register but it is fine.

echesakov

LGTM

test

21cce59

sandreenko added the NO-REVIEW Experimental/testing PR, do NOT review it label Jan 13, 2021

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 13, 2021

Add x86 support in genSIMDSplitReturn.

8cda4fe

sandreenko marked this pull request as ready for review January 15, 2021 00:50

sandreenko removed the NO-REVIEW Experimental/testing PR, do NOT review it label Jan 15, 2021

sandreenko changed the title ~~test~~ x86: support Return SIMD8. Jan 15, 2021

sandreenko requested a review from echesakov January 15, 2021 00:53

jkoritzinsky approved these changes Jan 15, 2021

View reviewed changes

Sergey Andreenko added 2 commits January 14, 2021 18:22

Fix for old machines.

719e428

fix format.

5a474b2

sandreenko requested a review from davidwrighton January 15, 2021 18:40

tannergooding reviewed Jan 15, 2021

View reviewed changes

src/coreclr/jit/instr.cpp Outdated Show resolved Hide resolved

tannergooding reviewed Jan 15, 2021

View reviewed changes

src/coreclr/jit/codegenxarch.cpp Outdated Show resolved Hide resolved

response review.

a447e2f

JulieLeeMSFT assigned sandreenko Jan 16, 2021

JulieLeeMSFT added this to the 6.0.0 milestone Jan 16, 2021

jkoritzinsky mentioned this pull request Jan 19, 2021

Remove extra UnmanagedCallersOnly overhead on x86 #46238

Closed

echesakov approved these changes Jan 19, 2021

View reviewed changes

sandreenko merged commit 5a91b0c into dotnet:master Jan 19, 2021

sandreenko deleted the genSIMDSplitReturn branch January 19, 2021 23:35

ghost locked as resolved and limited conversation to collaborators Feb 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x86: support Return SIMD8. #46899

x86: support Return SIMD8. #46899

sandreenko commented Jan 13, 2021 •

edited

Loading

sandreenko commented Jan 13, 2021

azure-pipelines bot commented Jan 13, 2021

sandreenko commented Jan 13, 2021

azure-pipelines bot commented Jan 13, 2021

sandreenko commented Jan 13, 2021

sandreenko commented Jan 15, 2021

jkoritzinsky left a comment

jkoritzinsky Jan 15, 2021

jkoritzinsky Jan 15, 2021 •

edited

Loading

sandreenko Jan 15, 2021

jkoritzinsky Jan 15, 2021 •

edited

Loading

echesakov Jan 15, 2021 •

edited

Loading

tannergooding Jan 15, 2021

jkotas Jan 15, 2021

davidwrighton Jan 15, 2021

sandreenko Jan 15, 2021

davidwrighton Jan 19, 2021

sandreenko commented Jan 15, 2021

echesakov left a comment

x86: support Return SIMD8. #46899

x86: support Return SIMD8. #46899

Conversation

sandreenko commented Jan 13, 2021 • edited Loading

sandreenko commented Jan 13, 2021

azure-pipelines bot commented Jan 13, 2021

sandreenko commented Jan 13, 2021

azure-pipelines bot commented Jan 13, 2021

sandreenko commented Jan 13, 2021

sandreenko commented Jan 15, 2021

jkoritzinsky left a comment

Choose a reason for hiding this comment

jkoritzinsky Jan 15, 2021

Choose a reason for hiding this comment

jkoritzinsky Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

sandreenko Jan 15, 2021

Choose a reason for hiding this comment

jkoritzinsky Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

echesakov Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

tannergooding Jan 15, 2021

Choose a reason for hiding this comment

jkotas Jan 15, 2021

Choose a reason for hiding this comment

davidwrighton Jan 15, 2021

Choose a reason for hiding this comment

sandreenko Jan 15, 2021

Choose a reason for hiding this comment

davidwrighton Jan 19, 2021

Choose a reason for hiding this comment

sandreenko commented Jan 15, 2021

echesakov left a comment

Choose a reason for hiding this comment

sandreenko commented Jan 13, 2021 •

edited

Loading

jkoritzinsky Jan 15, 2021 •

edited

Loading

jkoritzinsky Jan 15, 2021 •

edited

Loading

echesakov Jan 15, 2021 •

edited

Loading