Skip to content

Commit

Permalink
Fix optComputeLoopSideEffects to account for HWI stores (#61911)
Browse files Browse the repository at this point in the history
Otherwise we can end up not seeing the loop has memory havoc.

Also added an assert that will prevent this issue from arising in the future.
  • Loading branch information
SingleAccretion authored Nov 29, 2021
1 parent d9a5789 commit 6069861
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 1 deletion.
11 changes: 10 additions & 1 deletion src/coreclr/jit/optimizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7539,6 +7539,15 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)
}
break;

#ifdef FEATURE_HW_INTRINSICS
case GT_HWINTRINSIC:
if (tree->AsHWIntrinsic()->OperIsMemoryStore())
{
memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed);
}
break;
#endif // FEATURE_HW_INTRINSICS

case GT_LOCKADD:
case GT_XORR:
case GT_XAND:
Expand All @@ -7547,7 +7556,6 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)
case GT_CMPXCHG:
case GT_MEMORYBARRIER:
{
assert(!tree->OperIs(GT_LOCKADD) && "LOCKADD should not appear before lowering");
memoryHavoc |= memoryKindSet(GcHeap, ByrefExposed);
}
break;
Expand Down Expand Up @@ -7587,6 +7595,7 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)

default:
// All other gtOper node kinds, leave 'memoryHavoc' unchanged (i.e. false)
assert(!tree->OperRequiresAsgFlag());
break;
}
}
Expand Down
73 changes: 73 additions & 0 deletions src/tests/JIT/opt/Loops/LoopSideEffectsForHwiStores.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.X86;
using System.Runtime.Intrinsics.Arm;

unsafe class LoopSideEffectsForHwiStores
{
public static int Main()
{
static bool VerifyExpectedVtor(Vector128<int> a) => a.Equals(Vector128.Create(4));

var a = new ClassWithVtor();

fixed (Vector128<int>* p = &a.VtorField)
{
if (Sse2.IsSupported && !VerifyExpectedVtor(ProblemWithSse2(a, (byte*)p)))
{
System.Console.WriteLine("ProblemWithSse2 failed!");
return 101;
}

if (AdvSimd.IsSupported && !VerifyExpectedVtor(ProblemWithAdvSimd(a, (byte*)p)))
{
System.Console.WriteLine("ProblemWithAdvSimd failed!");
return 101;
}
}

return 100;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static unsafe Vector128<int> ProblemWithSse2(ClassWithVtor a, byte* p)
{
Vector128<int> vtor = Vector128<int>.Zero;

a.VtorField = Vector128.Create(1);
a.VtorField = Sse2.Add(a.VtorField, a.VtorField);

for (int i = 0; i < 10; i++)
{
vtor = Sse2.Add(vtor, Sse2.Add(a.VtorField, a.VtorField));
Sse2.Store(p, Vector128<byte>.Zero);
}

return vtor;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static unsafe Vector128<int> ProblemWithAdvSimd(ClassWithVtor a, byte* p)
{
Vector128<int> vtor = Vector128<int>.Zero;

a.VtorField = Vector128.Create(1);
a.VtorField = AdvSimd.Add(a.VtorField, a.VtorField);

for (int i = 0; i < 10; i++)
{
vtor = AdvSimd.Add(vtor, AdvSimd.Add(a.VtorField, a.VtorField));
AdvSimd.Store(p, Vector128<byte>.Zero);
}

return vtor;
}

class ClassWithVtor
{
public Vector128<int> VtorField;
}
}
14 changes: 14 additions & 0 deletions src/tests/JIT/opt/Loops/LoopSideEffectsForHwiStores.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<CLRTestPriority>1</CLRTestPriority>
</PropertyGroup>
<PropertyGroup>
<DebugType>PdbOnly</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit 6069861

Please sign in to comment.