Skip to content
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.

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

[JIT] Fix - Do not remove CAST nodes on store if the LCL_VAR is address exposed #85734

Merged
merged 8 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -1093,6 +1093,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.
Comment on lines +1097 to +1103
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this comment is formatted strangely, missing some spaces and it should be "address-exposed", not "address - exposed".

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 @@ -10115,8 +10115,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.
Comment on lines +10122 to +10128
Copy link
Member

Choose a reason for hiding this comment

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

The formatting here is a bit strange, maybe reformat the comment locally?

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>