Skip to content

Allow SIMD-returning calls as arguments #74184

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

Merged
merged 2 commits into from
Aug 24, 2022
Merged

Allow SIMD-returning calls as arguments #74184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ace56a3
Add a test
SingleAccretion Aug 18, 2022
f91c6ab
Allow SIMD-returning calls as arguments
SingleAccretion Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1419,7 +1419,7 @@ GenTree* Lowering::LowerFloatArg(GenTree** pArg, CallArg* callArg)
break;
}
GenTree* node = use.GetNode();
if (varTypeIsFloating(node))
if (varTypeUsesFloatReg(node))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@SingleAccretion SingleAccretion Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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".

{
GenTree* intNode = LowerFloatArgReg(node, currRegNumber);
assert(intNode != nullptr);
Expand All @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

{
GenTree* intNode = LowerFloatArgReg(arg, callArg->AbiInfo.GetRegNum());
assert(intNode != nullptr);
Expand All @@ -1464,11 +1464,13 @@ GenTree* Lowering::LowerFloatArg(GenTree** pArg, CallArg* callArg)
//
GenTree* Lowering::LowerFloatArgReg(GenTree* arg, regNumber regNum)
{
assert(varTypeUsesFloatReg(arg));

var_types floatType = arg->TypeGet();
assert(varTypeIsFloating(floatType));
var_types intType = (floatType == TYP_DOUBLE) ? TYP_LONG : TYP_INT;
GenTree* intArg = comp->gtNewBitCastNode(intType, arg);
var_types intType = (floatType == TYP_FLOAT) ? TYP_INT : TYP_LONG;
GenTree* intArg = comp->gtNewBitCastNode(intType, arg);
intArg->SetRegNum(regNum);

#ifdef TARGET_ARM
if (floatType == TYP_DOUBLE)
{
Expand Down Expand Up @@ -3828,6 +3830,11 @@ void Lowering::LowerCallStruct(GenTreeCall* call)
assert(user->TypeIs(origType) || (returnType == user->TypeGet()));
break;

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

Comment on lines +3833 to +3837
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

case GT_STOREIND:
#ifdef FEATURE_SIMD
if (varTypeIsSIMD(user))
Expand Down
82 changes: 82 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_74126/Runtime_74126.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Numerics;
using System.Runtime.Intrinsics;
using System.Runtime.CompilerServices;

public class Runtime_74126
{
public static int Main()
{
if (GetVtor(GetVtor2()) != GetVtor2())
{
return 101;
}
if (GetVtor(GetVtor3()) != GetVtor3())
{
return 102;
}
if (GetVtor(GetVtor4()) != GetVtor4())
{
return 103;
}
if (GetVtor(GetVtor64()) != GetVtor64())
{
return 104;
}
if (GetVtor(GetVtor128()) != GetVtor128())
{
return 105;
}
if (GetVtor(GetVtor256()) != GetVtor256())
{
return 106;
}

return 100;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static Vector2 GetVtor2()
{
return new Vector2(1, 2);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static Vector3 GetVtor3()
{
return new Vector3(1, 2, 3);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static Vector4 GetVtor4()
{
return new Vector4(1, 2, 3, 4);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static Vector64<int> GetVtor64()
{
return Vector64.Create(1, 2);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static Vector128<int> GetVtor128()
{
return Vector128.Create(1, 2, 3, 4);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static Vector256<int> GetVtor256()
{
return Vector256.Create(1, 2, 3, 4, 5, 6, 7, 8);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static T GetVtor<T>(T vtor)
{
return vtor;
}
}
9 changes: 9 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_74126/Runtime_74126.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>