Skip to content

ARM64-SVE: Avoid containing non-embedded conditional select #105719 #105812

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 8 commits into from
Aug 8, 2024
Merged

ARM64-SVE: Avoid containing non-embedded conditional select #105719 #105812

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7217ce7
ARM64-SVE: Avoid containing non-embedded conditional select
a74nh Aug 1, 2024
c687825
Add test
a74nh Aug 5, 2024
29cadd5
Minimal test csproj file
a74nh Aug 5, 2024
37ac806
Add NoWarn
a74nh Aug 5, 2024
b86f18b
Remove conditionalselect
a74nh Aug 6, 2024
2d4d006
Fix formatting
a74nh Aug 6, 2024
f76d1d2
Add lowerarmarch.cpp to SVE CI testing
a74nh Aug 7, 2024
cd7cd94
More SVE CI testing entries
a74nh Aug 7, 2024
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
4 changes: 2 additions & 2 deletions src/coreclr/jit/lowerarmarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3919,11 +3919,11 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}

// Handle op3
if (op3->IsVectorZero() && op1->IsMaskAllBitsSet())
if (op3->IsVectorZero() && op1->IsMaskAllBitsSet() && op2->IsEmbMaskOp())
{
// When we are merging with zero, we can specialize
// and avoid instantiating the vector constant.
// Do this only if op1 was AllTrueMask
// Do this only if op1 was AllTrueMask and op2 is embedded.
MakeSrcContained(node, op3);
}
Copy link
Member

@tannergooding tannergooding Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is the right fix. The zero (and in fact any constant or other value here) is still containable.

op1 is AllBitsSet so therefore nothing from op3 can ever be selected, so it is "unused". This means it is valid to drop the sel entirely and just emit op2 directly.

The only time we should be hitting this path is when op2 is an operation that requires a mask to be specified (even if unused) or some manually written user code in minopts. For the latter, it's still fine to contain the zero constant and just use the op2 register for both inputs (containment is a basic operation that happens at all codegen levels).

Copy link
Contributor Author

@a74nh a74nh Aug 6, 2024

Choose a reason for hiding this comment

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

op1 is AllBitsSet so therefore nothing from op3 can ever be selected, so it is "unused". This means it is valid to drop the sel entirely and just emit op2 directly.

Updated to do this instead.

Required the embedded HWIntrinsic check, otherwise lowering goes into an infinite loop adding and removing conditional selects.


Expand Down
31 changes: 31 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_105719/Runtime_105719.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

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

public class Runtime_105723
{
[Fact]
public static void TestEntryPoint()
{
if (Sve.IsSupported)
{
var vr3 = Sve.CreateTrueMaskInt32();
var vr4 = Vector.Create<int>(1);
var vr5 = Vector128.CreateScalar(0).AsVector();
Vector<int> vr6 = Sve.ConditionalSelect(vr3, vr4, vr5);
Consume(vr6);
}
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static void Consume(Vector<int> v)
{
;
}
}
12 changes: 12 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_105719/Runtime_105719.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<!-- Needed for CLRTestEnvironmentVariable -->
<RequiresProcessIsolation>true</RequiresProcessIsolation>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />

<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
</ItemGroup>
</Project>