Skip to content

Commit

Permalink
[JIT] Fix - Do not remove CAST nodes on store if the LCL_VAR is a…
Browse files Browse the repository at this point in the history
… parameter or struct field (#85734)

* Do not remove CAST nodes on assignment if the LCL_VAR is a parameter.

* Added NormalizeOnLoad rules from SingleAccretion. Added description of why we cannot remove CAST nodes from parameters.

* Remove morph optimization for NormalizeOnLoad in fgMorphLocalVar. Update test.

* Do not OptimizeCastOnStore for params and struct fields

* Update src/coreclr/jit/morph.cpp

Co-authored-by: Jakob Botsch Nielsen <[email protected]>

* Formatting

---------

Co-authored-by: Jakob Botsch Nielsen <[email protected]>
  • Loading branch information
TIHan and jakobbotsch authored Jul 20, 2023
1 parent 2ec8a68 commit 42322f5
Show file tree
Hide file tree
Showing 5 changed files with 129 additions and 3 deletions.
8 changes: 8 additions & 0 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -1096,6 +1096,14 @@ class LclVarDsc
assert(lvIsStructField);
return ((lvFldOffset % TARGET_POINTER_SIZE) == 0);
}

// NormalizeOnLoad Rules:
// 1. All small locals are actually TYP_INT locals.
// 2. NOL locals are such that not all definitions can be controlled by the compiler and so the upper bits can
// be undefined.For parameters this is the case because of ABI.For struct fields - because of padding.For
// address - exposed locals - because not all stores are direct.
// 3. Hence, all NOL uses(unless proven otherwise) are assumed in morph to have undefined upper bits and
// explicit casts have be inserted to "normalize" them back to conform to IL semantics.
bool lvNormalizeOnLoad() const
{
return varTypeIsSmall(TypeGet()) &&
Expand Down
16 changes: 14 additions & 2 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10078,8 +10078,20 @@ GenTree* Compiler::fgOptimizeCastOnStore(GenTree* store)
if (!src->OperIs(GT_CAST))
return store;

if (store->OperIs(GT_STORE_LCL_VAR) && !lvaGetDesc(store->AsLclVarCommon())->lvNormalizeOnLoad())
return store;
if (store->OperIs(GT_STORE_LCL_VAR))
{
LclVarDsc* varDsc = lvaGetDesc(store->AsLclVarCommon()->GetLclNum());

// We can make this transformation only under the assumption that NOL locals are always normalized before they
// are used,
// however this is not always the case: the JIT will utilize subrange assertions for NOL locals to make
// normalization
// assumptions -- see fgMorphLeafLocal. Thus we can only do this for cases where we know for sure that
// subsequent uses
// will normalize, which we can only guarantee when the local is address exposed.
if (!varDsc->lvNormalizeOnLoad() || !varDsc->IsAddressExposed())
return store;
}

if (src->gtOverflow())
return store;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public static int M8(byte arg0)
}
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/85081")]
[Fact]
public static int TestEntryPoint() {
var result = Test.Program.M8(1);
if (result != 255)
Expand Down
98 changes: 98 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// 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.Collections;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using Xunit;

public class Test
{
// This is trying to verify that we properly zero-extend on all platforms.
public class Program
{
public static long s_15;
public static sbyte s_17;
public static ushort s_21 = 36659;
public static int Test()
{
s_15 = ~1;
return M40(0);
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static void Consume(ushort x) { }

[MethodImpl(MethodImplOptions.NoInlining)]
public static int M40(ushort arg0)
{
for (int var0 = 0; var0 < 2; var0++)
{
arg0 = 65535;
arg0 &= (ushort)(s_15++ >> s_17);
arg0 %= s_21;
}

Consume(arg0);

if (arg0 != 28876)
{
return 0;
}
return 100;
}
}

public class Program2
{
public static long s_15;
public static sbyte s_17;
public static ushort s_21 = 36659;
public static int Test()
{
s_15 = ~1;
return M40();
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static void Consume(ushort x) { }

[MethodImpl(MethodImplOptions.NoInlining)]
public static int M40()
{
S s = default;
for (int var0 = 0; var0 < 2; var0++)
{
s.U = 65535;
s.U &= (ushort)(s_15++ >> s_17);
s.U %= s_21;
}

Consume(s.U);

if (s.U != 28876)
{
return 0;
}
return 100;
}

public struct S { public ushort U; }
}

[Fact]
public static int TestEntryPoint() {
if (Test.Program.Test() != 100)
{
return 0;
}

if (Test.Program2.Test() != 100)
{
return 0;
}

return 100;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit 42322f5

Please sign in to comment.