Allow SIMD-returning calls as arguments #74184

SingleAccretion · 2022-08-18T21:43:38Z

As of this change we handle all relevant ABI scenarios.

Windows x64:
- SIMD8: returned and passed as TYP_LONG, fine.
- SIMD12 / SIMD16 / SIMD32: returned and passed via a return buffer, fine.
Unix x64:
- SIMD8: returned and passed in one FP register, fine.
- SIMD12 / SIMD16, Vector4: returned and passed in two FP registers, fine.
- SIMD16, Vector128 / SIMD32: returned via a return buffer, passed on stack, fine.
x86:
- SIMD8: can be returned via two registers or a return buffer (and is always passed on stack), both are fine.
- SIMD12/SIMD16/SIMD32: returned via a return buffer, passed on stack, fine.
ARM64:
- SIMD8, Vector2: returned in two FP registers (and passed as such or TYP_LONG under Windows varargs), fine.
- SIMD8, Vector64: returned in one FP register, can be passed as such or as TYP_LONG under Windows varargs.
  The latter case is now handled correctly in Lowering::LowerArg.
- SIMD12: returned in three FP registers, passed as such or in two integer registers under Windows varargs, fine.
- SIMD16, Vector4: returned in four FP registers, passed as such, or in two integer registers under Windows varargs, fine.
- SIMD16, Vector128: returned in one FP register, passed as such, or in two integer registers under Windows varargs, fine
  (morph will decompose the varargs case into a FIELD_LIST via a temp).

Fixes #74126.

ghost · 2022-08-18T21:43:57Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

As of this change we handle all relevant ABI scenarios.

Windows x64:
- SIMD8: returned and passed as TYP_LONG, fine.
- SIMD12 / SIMD16 / SIMD32: returned and passed via a return buffer, fine.
Unix x64:
- SIMD8: returned and passed in one FP register, fine.
- SIMD12 / SIMD16, Vector4: returned and passed in two FP registers, fine.
- SIMD16, Vector128 / SIMD32: returned and passed via a return buffer, fine.
x86:
- SIMD8: can be returned via two registers or a return buffer (and is always passed on stack), both are fine.
- SIMD12/SIMD16/SIMD32: returned and passed via a return buffer, fine.
ARM64:
- SIMD8, Vector2: returned in two FP registers (and passed as such or TYP_LONG under Windows varargs), fine.
- SIMD8, Vector64: returned in one FP register, can be passed as such or as TYP_LONG under Windows varargs.
  The latter case is now handled correctly in Lowering::LowerArg.
- SIMD12: returned in three FP registers, passed as such or in two integer registers under Windows varargs, fine.
- SIMD16, Vector4: returned in four FP registers, passed as such, or in two integer registers under Windows varargs, fine.
- SIMD16, Vector128: returned in one FP register, passed as such, or in two integer registers under Windows varargs, fine
  (morph will decompose the varargs case into a FIELD_LIST via a temp).

Fixes #74126.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

As of this change we handle all relevant ABI scenarios. 1) Windows x64: - SIMD8: returned and passed as "TYP_LONG", fine. - SIMD12 / SIMD16 / SIMD32: returned and passed via a return buffer, fine. 2) Unix x64: - SIMD8: returned and passed in one FP register, fine. - SIMD12 / SIMD16, Vector4: returned and passed in two FP registers, fine. - SIMD16, Vector128 / SIMD32: returned and passed via a return buffer, fine. 3) x86: - SIMD8: can be returned via two registers or a return buffer (and is always passed on stack), both are fine. - SIMD12/SIMD16/SIMD32: returned via a return buffer, passed on stack, fine. 4) ARM64: - SIMD8, Vector2: returned in two FP registers (and passed as such or "TYP_LONG" under Windows varargs), fine. - SIMD8, Vector64: returned in one FP register, can be passed as such or as "TYP_LONG" under Windows varargs. The latter case is now handled correctly in "Lowering::LowerArg". - SIMD12: returned in three FP registers, passed as such or in two integer registers under Windows varargs, fine. - SIMD16, Vector4: returned in four FP registers, passed as such, or in two integer registers under Windows varargs, fine. - SIMD16, Vector128: returned in one FP register, passed as such, or in two integer registers under Windows varargs, fine (morph will decompose the varargs case into a `FIELD_LIST` via a temp).

SingleAccretion · 2022-08-21T16:22:10Z

SPMI failure is pre-existing (empty OSX ARM64 contexts?), Installer Build and Test coreclr Linux_musl_arm64 Debug failure is infrastructure-related:

git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --prune-tags --progress --no-recurse-submodules origin --depth=20 +2cbdbb3a894090a60d09337496df7943a38a10ab
error: RPC failed; curl 18 transfer closed with outstanding read data remaining
fatal: expected flush after ref listing

No diffs.

I am assuming we'll want to backport this to 7.0.

JulieLeeMSFT · 2022-08-22T21:29:33Z

@BruceForstall, please triage a milestone.

BruceForstall · 2022-08-22T21:40:17Z

cc @tannergooding @dotnet/jit-contrib

tannergooding · 2022-08-22T21:45:35Z

Windows x64:
SIMD8: returned and passed as TYP_LONG, fine.
SIMD12 / SIMD16 / SIMD32: returned and passed via a return buffer, fine.

Just noting that SIMD16/SIMD32 being returned via an output buffer is an "it depends" scenario.

While Vector4 is a TYP_SIMD16 and a "user-defined struct" and therefore should be returned via an output buffer, Vector128<T> and Vector256<T> are the __m128 and __m256 types and are actually returned in XMM0/YMM0: https://docs.microsoft.com/en-us/cpp/build/x64-calling-convention?view=msvc-170#return-values (YMM0 isn't explicitly listed, but checking the MSVC output for such a scenario shows its the case).

Vector<T> doesn't have a well-defined ABI and is "managed only". We're likely better off, codegen wise, treating it like __m128/__m256 and also having it return in register.

src/coreclr/jit/lower.cpp

BruceForstall

Just a couple question

BruceForstall · 2022-08-24T19:11:01Z

src/coreclr/jit/lower.cpp

@@ -1419,7 +1419,7 @@ GenTree* Lowering::LowerFloatArg(GenTree** pArg, CallArg* callArg)
                    break;
                }
                GenTree* node = use.GetNode();
-                if (varTypeIsFloating(node))
+                if (varTypeUsesFloatReg(node))


Should this be varTypeUsesFloatArgReg()? Effectively it's no difference, except for LoongArch64.

Unrelated, but the code below seems odd:

if (node->TypeGet() == TYP_DOUBLE) { currRegNumber = REG_NEXT(REG_NEXT(currRegNumber)); regIndex += 2; } else { currRegNumber = REG_NEXT(currRegNumber); regIndex += 1; }

I would expect the TYPE_DOUBLE == 2 registers code to only apply to arm32, but it's not ifdef'ed that way.

I would expect the TYPE_DOUBLE == 2 registers code to only apply to arm32, but it's not ifdef'ed that way.

Indeed, I thought the same but decided against #ifdefing it in this change to keep the scope down.

It so happens that we will never have DOUBLE here on ARM64 (which is the case of interest) because morph will construct the FIELD_LIST with LONGs. Looking what happens under Linux ARM soft FP, I see the same, so it seems likely that this code is actually dead.

Should this be varTypeUsesFloatArgReg() ?

I'd think it's better with varTypeUsesFloatReg. varTypeUsesFloatArgReg has the meaning of "can this type be used as an argument from an FP register file", while here we're asking the question of "does this node define an FP register".

BruceForstall · 2022-08-24T19:11:09Z

src/coreclr/jit/lower.cpp

@@ -1441,7 +1441,7 @@ GenTree* Lowering::LowerFloatArg(GenTree** pArg, CallArg* callArg)
            // List fields were replaced in place.
            return arg;
        }
-        else if (varTypeIsFloating(arg))
+        else if (varTypeUsesFloatReg(arg))


BruceForstall · 2022-08-24T20:14:57Z

Another question: this change is only in armarch (and LoongArch) code. But #74126 is a failure in win-x64. So how does this fix #74126?

SingleAccretion · 2022-08-24T20:17:55Z

src/coreclr/jit/lower.cpp

+            case GT_CALL:
+                // Argument lowering will deal with register file mismatches if needed.
+                assert(varTypeIsSIMD(origType));
+                break;
+


@BruceForstall it's this change that fixes the original failure.

BruceForstall · 2022-08-24T20:24:24Z

/backport to release/7.0

github-actions · 2022-08-24T20:24:35Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2922063287

BruceForstall · 2022-08-24T20:29:26Z

@SingleAccretion Feel free to amend #74520 with additional justification for porting the fix back to 7.0

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 18, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 18, 2022

SingleAccretion added 2 commits August 19, 2022 15:06

Add a test

ace56a3

SingleAccretion force-pushed the Simd-Calls-As-Args branch from 124cb3e to f91c6ab Compare August 19, 2022 12:07

runfoapp bot mentioned this pull request Aug 19, 2022

Methodical_others test JIT/Methodical/Coverage/copy_prop_byref_to_native_int crashing #69832

Open

SingleAccretion closed this Aug 21, 2022

SingleAccretion reopened this Aug 21, 2022

SingleAccretion marked this pull request as ready for review August 21, 2022 16:22

JulieLeeMSFT assigned SingleAccretion Aug 22, 2022

JulieLeeMSFT requested a review from BruceForstall August 22, 2022 21:28

JulieLeeMSFT assigned BruceForstall Aug 22, 2022

BruceForstall added this to the 7.0.0 milestone Aug 22, 2022

tannergooding reviewed Aug 24, 2022

View reviewed changes

src/coreclr/jit/lower.cpp Show resolved Hide resolved

tannergooding approved these changes Aug 24, 2022

View reviewed changes

BruceForstall reviewed Aug 24, 2022

View reviewed changes

SingleAccretion commented Aug 24, 2022

View reviewed changes

BruceForstall approved these changes Aug 24, 2022

View reviewed changes

BruceForstall merged commit a07f2e9 into dotnet:main Aug 24, 2022

SingleAccretion deleted the Simd-Calls-As-Args branch August 24, 2022 20:24

github-actions bot mentioned this pull request Aug 24, 2022

[release/7.0] Allow SIMD-returning calls as arguments #74520

Merged

ghost locked as resolved and limited conversation to collaborators Sep 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow SIMD-returning calls as arguments #74184

Allow SIMD-returning calls as arguments #74184

SingleAccretion commented Aug 18, 2022 •

edited

Loading

ghost commented Aug 18, 2022

SingleAccretion commented Aug 21, 2022

JulieLeeMSFT commented Aug 22, 2022

BruceForstall commented Aug 22, 2022

tannergooding commented Aug 22, 2022 •

edited

Loading

BruceForstall left a comment

BruceForstall Aug 24, 2022

SingleAccretion Aug 24, 2022 •

edited

Loading

BruceForstall Aug 24, 2022

BruceForstall commented Aug 24, 2022

SingleAccretion Aug 24, 2022

BruceForstall commented Aug 24, 2022

github-actions bot commented Aug 24, 2022

BruceForstall commented Aug 24, 2022

Allow SIMD-returning calls as arguments #74184

Allow SIMD-returning calls as arguments #74184

Conversation

SingleAccretion commented Aug 18, 2022 • edited Loading

ghost commented Aug 18, 2022

SingleAccretion commented Aug 21, 2022

JulieLeeMSFT commented Aug 22, 2022

BruceForstall commented Aug 22, 2022

tannergooding commented Aug 22, 2022 • edited Loading

BruceForstall left a comment

Choose a reason for hiding this comment

BruceForstall Aug 24, 2022

Choose a reason for hiding this comment

SingleAccretion Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

BruceForstall Aug 24, 2022

Choose a reason for hiding this comment

BruceForstall commented Aug 24, 2022

SingleAccretion Aug 24, 2022

Choose a reason for hiding this comment

BruceForstall commented Aug 24, 2022

github-actions bot commented Aug 24, 2022

BruceForstall commented Aug 24, 2022

SingleAccretion commented Aug 18, 2022 •

edited

Loading

tannergooding commented Aug 22, 2022 •

edited

Loading

SingleAccretion Aug 24, 2022 •

edited

Loading