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.

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

Forbid - byref cnst -> + (byref -cnst) transformation. #44266

Merged
merged 6 commits into from
Nov 6, 2020
Merged

Forbid - byref cnst -> + (byref -cnst) transformation. #44266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
546cc65
Add a repro test.
Nov 5, 2020
660dad6
Forbid the transformation for byrefs.
Nov 5, 2020
db8b3ff
Update src/coreclr/src/jit/morph.cpp
Nov 5, 2020
6318887
Update src/coreclr/src/jit/morph.cpp
AndyAyersMS Nov 5, 2020
8d1d5d4
Fix the test return value.
Nov 5, 2020
f81f6c9
improve the test.
Nov 5, 2020
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
5 changes: 3 additions & 2 deletions src/coreclr/src/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13354,9 +13354,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
/* Check for "op1 - cns2" , we change it to "op1 + (-cns2)" */

noway_assert(op2);
if (op2->IsCnsIntOrI())
if (op2->IsCnsIntOrI() && !op2->IsIconHandle())
{
/* Negate the constant and change the node to be "+" */
// Negate the constant and change the node to be "+",
// except when `op2` is a const byref.

op2->AsIntConCommon()->SetIconValue(-op2->AsIntConCommon()->IconValue());
op2->AsIntConRef().gtFieldSeq = FieldSeqStore::NotAField();
Expand Down
63 changes: 63 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// This test shows an inlining of `byref LCL_VAR_ADDR - byref CNST_INT` method that returns a native int.
// However, Jit could try to optimize `-` as `+ -CNST_INT` that could lead to an incorrect `long + (-byref)`.

.assembly extern System.Console {}
.assembly extern legacy library mscorlib {}
.assembly byrefsubbyref1 { }
.class a extends [mscorlib]System.Object
{
.field static class ctest S_1
.method public static native int byrefsubbyref(class ctest& V_1, class ctest& V_2) aggressiveinlining
{
ldarg 0
ldarg 1
sub
ret
}

.method public static int32 main() cil managed
{
.entrypoint
.maxstack 2
.locals init (class ctest V_1,
class ctest V_2,
native int V_3)
newobj instance void ctest::.ctor()
stloc.0
newobj instance void ctest::.ctor()
dup
stsfld class ctest a::S_1
stloc.1

ldloca V_1
ldsflda class ctest a::S_1
call native int a::byrefsubbyref(class ctest&, class ctest&)
stloc V_3
ldloc V_3
call void a::KeepA(native int)
ldc.i4.s 100
ret
}

.method public hidebysig static void KeepA(native int a) cil managed noinlining
{
.maxstack 8
ret
Copy link
Member

@MichalStrehovsky MichalStrehovsky Nov 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do you need conv.i4 before returning the value? Stack has native int, but method returns int32.

If you need the method to return 100, you could allocate a 101 element array and subtract the managed reference between the first element and last element.

The difference between a stack location and a static field that this is returning right now is a very random number.

Copy link
Contributor Author

@sandreenko sandreenko Nov 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The difference between a stack location and a static field that this is returning right now is a very random number.

You are right, I will fix the test to always return 100 instead of a random number.

I am not an expert in ECMA so I am very glad that you are looking at the test, thank you.
My understanding is that ECMA allows implicit coercion of native int to int so I do not need a cast there.
However, I could read it wrong and can add a cast just for safety.

from ECMA, page 38:
I.8.7.3 General assignment compatibility:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that @sandreenko is correct that this implicit conversion is allowed.

}
}

.class private auto ansi ctest
extends [mscorlib]System.Object
{
.method public specialname rtspecialname
instance void .ctor() cil managed
{
.maxstack 1
ldarg.0
call instance void [mscorlib]System.Object::.ctor()
ret
}
}
10 changes: 10 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.ilproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<Project Sdk="Microsoft.NET.Sdk.IL">
<PropertyGroup>
<OutputType>Exe</OutputType>
<DebugType>None</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).il" />
</ItemGroup>
</Project>