From d786e3df5d33f81ce71bc7a138c17c75cd8ab348 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 14 Apr 2022 20:03:50 -0700 Subject: [PATCH 1/4] JIT: fix flags lossage in morph comma/ind interchange Morph may transform `IND(COMMA(..., z))` into `COMMA(... , IND(z))` and when doing so it was not computing appropriately conservative flags for the new `IND`. In particular it was losing `GTF_GLOB_REF`. This allowed subsequent opts to reorder operands in an unsafe manner. Fixes #68049. --- src/coreclr/jit/morph.cpp | 7 +- .../JitBlue/Runtime_68049/Runtime_68049_0.cs | 70 ++++++++++++++++ .../Runtime_68049/Runtime_68049_0.csproj | 9 +++ .../JitBlue/Runtime_68049/Runtime_68049_1.cs | 81 +++++++++++++++++++ .../Runtime_68049/Runtime_68049_1.csproj | 9 +++ 5 files changed, 174 insertions(+), 2 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_0.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_0.csproj create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.csproj diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 99af642fd6c52a..e9a25c264d6262 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12991,9 +12991,12 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) GenTree* addr = commaNode->AsOp()->gtOp2; // TODO-1stClassStructs: we often create a struct IND without a handle, fix it. op1 = gtNewIndir(typ, addr); - // This is very conservative - op1->gtFlags |= treeFlags & ~GTF_ALL_EFFECT & ~GTF_IND_NONFAULTING; + + // Determine flags on the indir + // + op1->gtFlags |= treeFlags & ~GTF_ALL_EFFECT; op1->gtFlags |= (addr->gtFlags & GTF_ALL_EFFECT); + op1->gtFlags |= treeFlags & (GTF_GLOB_REF | GTF_IND_FLAGS); #ifdef DEBUG op1->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_0.cs b/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_0.cs new file mode 100644 index 00000000000000..5befba0fc1ab84 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_0.cs @@ -0,0 +1,70 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Generated by Fuzzlyn v1.5 on 2022-04-13 11:38:00 +// Run on X64 Linux +// Seed: 1784259920377383051 +// Reduced from 110.5 KiB to 0.9 KiB in 00:00:57 +// Debug: Outputs 1 +// Release: Outputs 0 + +public struct S0 +{ + public uint F0; + public long F1; + public S0(uint f0): this() + { + F0 = f0; + } + + public ulong M5() + { + var vr1 = new ushort[]{0}; + M6(vr1); + return 1; + } + + public void M6(ushort[] arg0) + { + this = new S0(0); + } +} + +public class Runtime_68049_0 +{ + public static long s_result; + public static IRuntime s_rt; + public static int Main() + { + s_rt = new Runtime(); + var vr4 = new S0[]{new S0(1)}; + var vr5 = new short[]{0}; + bool vr6 = M1(vr4, vr5) <= 1; + return (int)s_result; + } + + public static short M1(S0[] arg0, short[] arg1) + { + long var3 = arg0[0].F1; + var3 = (arg0[0].F0 & (byte)arg0[0].M5()); + s_rt.WriteLine(var3); + return arg1[0]; + } +} + +public interface IRuntime +{ + void WriteLine(T value); +} + +public class Runtime : IRuntime +{ + public void WriteLine(T value) + { + System.Console.WriteLine(value); + if (typeof(T) == typeof(long)) + { + Runtime_68049_0.s_result = ((long)(object)value) + 99; + } + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_0.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_0.csproj new file mode 100644 index 00000000000000..f492aeac9d056b --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_0.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.cs b/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.cs new file mode 100644 index 00000000000000..b7bcd772a6eabc --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.cs @@ -0,0 +1,81 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Generated by Fuzzlyn v1.5 on 2022-04-13 11:57:44 +// Run on X64 Windows +// Seed: 15524766127402705757 +// Reduced from 138.3 KiB to 1.6 KiB in 00:02:03 +// Debug: Outputs 1 +// Release: Outputs 254 + +public struct S0 +{ + public short F0; + public int F3; + public byte M29(ref sbyte[] arg0) + { + if (this.F0 > this.F3) + { + this.F0 = (short)~this.F0; + sbyte[, ] var0 = new sbyte[, ]{{1}}; + } + + return Runtime_68049_1.M31(); + } +} + +public class Runtime_68049_1 +{ + public static byte s_result; + public static IRuntime s_rt; + public static short[] s_2 = new short[]{0}; + public static sbyte[][] s_8 = new sbyte[][]{new sbyte[]{1}}; + public static byte s_9 = 28; + public static int Main() + { + CollectibleALC alc = new CollectibleALC(); + System.Reflection.Assembly asm = alc.LoadFromAssemblyPath(System.Reflection.Assembly.GetExecutingAssembly().Location); + System.Reflection.MethodInfo mi = asm.GetType(typeof(Runtime_68049_1).FullName).GetMethod(nameof(MainInner)); + System.Type runtimeTy = asm.GetType(typeof(Runtime).FullName); + mi.Invoke(null, new object[]{System.Activator.CreateInstance(runtimeTy)}); + return (int) s_result; + } + + public static void MainInner(IRuntime rt) + { + s_rt = rt; + S0[] vr2 = new S0[]{new S0()}; + s_2[0] = vr2[0].F0++; + byte vr3 = (byte)(vr2[0].F0 % (long)vr2[0].M29(ref s_8[0])); + s_rt.WriteLine("c_747", vr3); + } + + public static byte M31() + { + return s_9; + } +} + +public interface IRuntime +{ + void WriteLine(string site, T value); +} + +public class Runtime : IRuntime +{ + public void WriteLine(string site, T value) + { + System.Console.WriteLine(value); + if (typeof(T) == typeof(byte)) + { + Runtime_68049_1.s_result = (byte)(((byte)(object) value) + 99); + } + } +} + +public class CollectibleALC : System.Runtime.Loader.AssemblyLoadContext +{ + public CollectibleALC(): base(true) + { + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.csproj new file mode 100644 index 00000000000000..f492aeac9d056b --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + \ No newline at end of file From 1bd4b2449ec78ca4f3a00029c05bce4609ff1ed6 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 15 Apr 2022 12:02:59 -0700 Subject: [PATCH 2/4] feedback --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index e9a25c264d6262..b55e445a6f23f1 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12996,7 +12996,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // op1->gtFlags |= treeFlags & ~GTF_ALL_EFFECT; op1->gtFlags |= (addr->gtFlags & GTF_ALL_EFFECT); - op1->gtFlags |= treeFlags & (GTF_GLOB_REF | GTF_IND_FLAGS); + op1->gtFlags |= treeFlags & GTF_GLOB_REF; #ifdef DEBUG op1->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; From 9b87f0f845c34a0030f5131260a143115e5e4f43 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 15 Apr 2022 12:42:08 -0700 Subject: [PATCH 3/4] remove ALC-dependent test --- .../JitBlue/Runtime_68049/Runtime_68049_1.cs | 81 ------------------- .../Runtime_68049/Runtime_68049_1.csproj | 9 --- 2 files changed, 90 deletions(-) delete mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.cs delete mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.cs b/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.cs deleted file mode 100644 index b7bcd772a6eabc..00000000000000 --- a/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.cs +++ /dev/null @@ -1,81 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -// Generated by Fuzzlyn v1.5 on 2022-04-13 11:57:44 -// Run on X64 Windows -// Seed: 15524766127402705757 -// Reduced from 138.3 KiB to 1.6 KiB in 00:02:03 -// Debug: Outputs 1 -// Release: Outputs 254 - -public struct S0 -{ - public short F0; - public int F3; - public byte M29(ref sbyte[] arg0) - { - if (this.F0 > this.F3) - { - this.F0 = (short)~this.F0; - sbyte[, ] var0 = new sbyte[, ]{{1}}; - } - - return Runtime_68049_1.M31(); - } -} - -public class Runtime_68049_1 -{ - public static byte s_result; - public static IRuntime s_rt; - public static short[] s_2 = new short[]{0}; - public static sbyte[][] s_8 = new sbyte[][]{new sbyte[]{1}}; - public static byte s_9 = 28; - public static int Main() - { - CollectibleALC alc = new CollectibleALC(); - System.Reflection.Assembly asm = alc.LoadFromAssemblyPath(System.Reflection.Assembly.GetExecutingAssembly().Location); - System.Reflection.MethodInfo mi = asm.GetType(typeof(Runtime_68049_1).FullName).GetMethod(nameof(MainInner)); - System.Type runtimeTy = asm.GetType(typeof(Runtime).FullName); - mi.Invoke(null, new object[]{System.Activator.CreateInstance(runtimeTy)}); - return (int) s_result; - } - - public static void MainInner(IRuntime rt) - { - s_rt = rt; - S0[] vr2 = new S0[]{new S0()}; - s_2[0] = vr2[0].F0++; - byte vr3 = (byte)(vr2[0].F0 % (long)vr2[0].M29(ref s_8[0])); - s_rt.WriteLine("c_747", vr3); - } - - public static byte M31() - { - return s_9; - } -} - -public interface IRuntime -{ - void WriteLine(string site, T value); -} - -public class Runtime : IRuntime -{ - public void WriteLine(string site, T value) - { - System.Console.WriteLine(value); - if (typeof(T) == typeof(byte)) - { - Runtime_68049_1.s_result = (byte)(((byte)(object) value) + 99); - } - } -} - -public class CollectibleALC : System.Runtime.Loader.AssemblyLoadContext -{ - public CollectibleALC(): base(true) - { - } -} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.csproj deleted file mode 100644 index f492aeac9d056b..00000000000000 --- a/src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.csproj +++ /dev/null @@ -1,9 +0,0 @@ - - - Exe - True - - - - - \ No newline at end of file From dfb0840aebed91744637f1faff8640501cb97a7b Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 15 Apr 2022 17:35:40 -0700 Subject: [PATCH 4/4] revise flag setting once again --- src/coreclr/jit/morph.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index b55e445a6f23f1..f19b8a41c39f41 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12992,10 +12992,19 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // TODO-1stClassStructs: we often create a struct IND without a handle, fix it. op1 = gtNewIndir(typ, addr); - // Determine flags on the indir + // Determine flags on the indir. // op1->gtFlags |= treeFlags & ~GTF_ALL_EFFECT; op1->gtFlags |= (addr->gtFlags & GTF_ALL_EFFECT); + + // if this was a non-faulting indir, clear GTF_EXCEPT, + // unless we inherit it from the addr. + // + if (((treeFlags & GTF_IND_NONFAULTING) != 0) && ((addr->gtFlags & GTF_EXCEPT) == 0)) + { + op1->gtFlags &= ~GTF_EXCEPT; + } + op1->gtFlags |= treeFlags & GTF_GLOB_REF; #ifdef DEBUG