From 80f398dcae5a568bf18c115326e1199a6981edb9 Mon Sep 17 00:00:00 2001 From: Benjamin Hodgson Date: Sun, 5 Sep 2021 19:01:01 -0400 Subject: [PATCH 1/8] Better codegen for `(T)x | (T)y` I added a morph pass to fold expressions like `(T)x | (T)y` into `(T)(x | y)`. This results in fewer `movzx` instructions in the asm. Fixes #13816 --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/morph.cpp | 65 +++++++++++++++++++ .../JIT/CodeGenBringUpTests/CastThenBinop.cs | 38 +++++++++++ .../CastThenBinop_d.csproj | 13 ++++ .../CastThenBinop_do.csproj | 13 ++++ .../CastThenBinop_r.csproj | 13 ++++ .../CastThenBinop_ro.csproj | 13 ++++ 7 files changed, 156 insertions(+) create mode 100644 src/tests/JIT/CodeGenBringUpTests/CastThenBinop.cs create mode 100644 src/tests/JIT/CodeGenBringUpTests/CastThenBinop_d.csproj create mode 100644 src/tests/JIT/CodeGenBringUpTests/CastThenBinop_do.csproj create mode 100644 src/tests/JIT/CodeGenBringUpTests/CastThenBinop_r.csproj create mode 100644 src/tests/JIT/CodeGenBringUpTests/CastThenBinop_ro.csproj diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a247bff0907207..5d3268818797b4 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6157,6 +6157,7 @@ class Compiler GenTreeLclVar* fgMorphTryFoldObjAsLclVar(GenTreeObj* obj); GenTree* fgMorphCommutative(GenTreeOp* tree); + GenTree* fgMorphOpCast(GenTreeOp* tree); public: GenTree* fgMorphTree(GenTree* tree, MorphAddrContext* mac = nullptr); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index fac9b78d724b09..b3f5ba984cfeaf 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11053,6 +11053,60 @@ GenTree* Compiler::fgMorphCommutative(GenTreeOp* tree) return op1; } +//------------------------------------------------------------------------------ +// fgMorphOpCast : Try to simplify "(T)x op (T)y" to "(T)(x op y)". +// https://github.com/dotnet/runtime/issues/13816 +// +// Arguments: +// tree - node to fold +// +// return value: +// A folded GenTree* instance, or the same GenTree* if it couldn't be folded +// + +GenTree* Compiler::fgMorphOpCast(GenTreeOp* tree) +{ + assert(varTypeIsIntegralOrI(tree->TypeGet())); + assert(tree->OperIs(GT_OR, GT_AND, GT_XOR)); + + GenTree* op1 = tree->gtGetOp1(); + GenTree* op2 = tree->gtGetOp2(); + genTreeOps oper = tree->OperGet(); + + // see whether both ops are casts, with matching to and from types + if (op1->OperIs(GT_CAST) && op2->OperIs(GT_CAST)) + { + var_types fromType = op1->CastFromType(); + var_types toType = op1->CastToType(); + GenTreeFlags gtf_unsigned = op1->gtFlags & GTF_UNSIGNED; + + if (op2->CastFromType() != fromType || op2->CastToType() != toType || (op2->gtFlags & GTF_UNSIGNED) != gtf_unsigned) + { + return tree; + } + + // tree CAST + // / \ | + // CAST CAST ==> tree + // | | / \ + // op1 op2 op1 op2 + + tree->gtOp1 = op1->AsCast()->CastOp(); + tree->gtOp2 = op2->AsCast()->CastOp(); + tree->gtType = fromType; + + GenTree* result = gtNewCastNode(toType, tree, gtf_unsigned != 0, toType); + + DEBUG_DESTROY_NODE(op1); + DEBUG_DESTROY_NODE(op2); + INDEBUG(result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); + + return result; + } + + return tree; +} + /***************************************************************************** * * Transform the given GTK_SMPOP tree for code generation. @@ -12737,6 +12791,17 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) op2 = tree->AsOp()->gtOp2; } + if (varTypeIsIntegralOrI(tree->TypeGet()) && tree->OperIs(GT_AND, GT_OR, GT_XOR)) + { + tree = fgMorphOpCast(tree->AsOp()); + + // fgMorphOpCast may return a new tree + oper = tree->OperGet(); + typ = tree->TypeGet(); + op1 = tree->AsOp()->gtOp1; + op2 = tree->AsOp()->gtOp2; + } + if (varTypeIsIntegralOrI(tree->TypeGet()) && tree->OperIs(GT_ADD, GT_MUL, GT_AND, GT_OR, GT_XOR)) { GenTree* foldedTree = fgMorphCommutative(tree->AsOp()); diff --git a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop.cs b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop.cs new file mode 100644 index 00000000000000..2433094d28c24d --- /dev/null +++ b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop.cs @@ -0,0 +1,38 @@ +// 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; + +// Test for https://github.com/dotnet/runtime/issues/13816 +public class Test +{ + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + static int DowncastOr(int a, int b) + { + return (byte)a | (byte)b; + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + static long UpcastAnd(int a, int b) + { + return (long)a & (long)b; + } + + public static int Main() + { + const int Pass = 100; + const int Fail = -1; + + if (DowncastOr(0x0F, 0xF0) != 0xFF) + { + return Fail; + } + if (UpcastAnd(0x0FF, 0xFF0) != 0xF0) + { + return Fail; + } + + return Pass; + } +} diff --git a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_d.csproj b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_d.csproj new file mode 100644 index 00000000000000..a6b32eb44172e5 --- /dev/null +++ b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_d.csproj @@ -0,0 +1,13 @@ + + + Exe + 1 + + + Full + False + + + + + diff --git a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_do.csproj b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_do.csproj new file mode 100644 index 00000000000000..0df64817015e73 --- /dev/null +++ b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_do.csproj @@ -0,0 +1,13 @@ + + + Exe + 1 + + + Full + True + + + + + diff --git a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_r.csproj b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_r.csproj new file mode 100644 index 00000000000000..2bd35b2343c0d5 --- /dev/null +++ b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_r.csproj @@ -0,0 +1,13 @@ + + + Exe + 1 + + + PdbOnly + False + + + + + diff --git a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_ro.csproj b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_ro.csproj new file mode 100644 index 00000000000000..86fc858f449929 --- /dev/null +++ b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_ro.csproj @@ -0,0 +1,13 @@ + + + Exe + 1 + + + PdbOnly + True + + + + + From cdec019d463276d2546e47e51d6894e951f0a7b7 Mon Sep 17 00:00:00 2001 From: Benjamin Hodgson Date: Mon, 6 Sep 2021 15:34:21 -0400 Subject: [PATCH 2/8] Code review updates * Rename function to fgMorphCastedBitwiseOp * Don't fold checked arithmetic * Reuse op1 node for return value * Don't run outside global morphing * Various code style and comment tweaks --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/morph.cpp | 61 ++++++++++++++++++++++---------------- 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 5d3268818797b4..9cb7e1d01ce6aa 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6157,7 +6157,7 @@ class Compiler GenTreeLclVar* fgMorphTryFoldObjAsLclVar(GenTreeObj* obj); GenTree* fgMorphCommutative(GenTreeOp* tree); - GenTree* fgMorphOpCast(GenTreeOp* tree); + GenTree* fgMorphCastedBitwiseOp(GenTreeOp* tree); public: GenTree* fgMorphTree(GenTree* tree, MorphAddrContext* mac = nullptr); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index b3f5ba984cfeaf..bce39b89079e40 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11054,54 +11054,63 @@ GenTree* Compiler::fgMorphCommutative(GenTreeOp* tree) } //------------------------------------------------------------------------------ -// fgMorphOpCast : Try to simplify "(T)x op (T)y" to "(T)(x op y)". -// https://github.com/dotnet/runtime/issues/13816 +// fgMorphCastedBitwiseOp : Try to simplify "(T)x op (T)y" to "(T)(x op y)". // // Arguments: -// tree - node to fold -// -// return value: -// A folded GenTree* instance, or the same GenTree* if it couldn't be folded +// tree - node to fold // - -GenTree* Compiler::fgMorphOpCast(GenTreeOp* tree) +// Return Value: +// A folded GenTree* instance, or the same GenTree* if it couldn't be folded +GenTree* Compiler::fgMorphCastedBitwiseOp(GenTreeOp* tree) { - assert(varTypeIsIntegralOrI(tree->TypeGet())); + assert(varTypeIsIntegralOrI(tree)); assert(tree->OperIs(GT_OR, GT_AND, GT_XOR)); GenTree* op1 = tree->gtGetOp1(); GenTree* op2 = tree->gtGetOp2(); genTreeOps oper = tree->OperGet(); - // see whether both ops are casts, with matching to and from types + // see whether both ops are casts, with matching to and from types. if (op1->OperIs(GT_CAST) && op2->OperIs(GT_CAST)) { - var_types fromType = op1->CastFromType(); - var_types toType = op1->CastToType(); - GenTreeFlags gtf_unsigned = op1->gtFlags & GTF_UNSIGNED; + // bail if either operand is a checked cast + if (op1->gtOverflowEx() || op2->gtOverflowEx()) + { + return tree; + } + + var_types fromType = op1->AsCast()->CastOp()->TypeGet(); + var_types toType = op1->AsCast()->gtCastType; + bool isUnsigned = op1->IsUnsigned(); - if (op2->CastFromType() != fromType || op2->CastToType() != toType || (op2->gtFlags & GTF_UNSIGNED) != gtf_unsigned) + if ((op2->CastFromType() != fromType) || (op2->CastToType() != toType) || (op2->IsUnsigned() != isUnsigned)) { return tree; } - // tree CAST + // Reuse gentree nodes: + // + // tree op1 // / \ | - // CAST CAST ==> tree + // op1 op2 ==> tree // | | / \ - // op1 op2 op1 op2 + // x y x y + // + // (op2 becomes garbage) tree->gtOp1 = op1->AsCast()->CastOp(); tree->gtOp2 = op2->AsCast()->CastOp(); tree->gtType = fromType; - GenTree* result = gtNewCastNode(toType, tree, gtf_unsigned != 0, toType); + op1->gtType = genActualType(toType); + op1->AsCast()->gtOp1 = tree; + op1->AsCast()->gtCastType = toType; + // no need to update isUnsigned - DEBUG_DESTROY_NODE(op1); DEBUG_DESTROY_NODE(op2); - INDEBUG(result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); + INDEBUG(op1->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); - return result; + return op1; } return tree; @@ -12791,15 +12800,15 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) op2 = tree->AsOp()->gtOp2; } - if (varTypeIsIntegralOrI(tree->TypeGet()) && tree->OperIs(GT_AND, GT_OR, GT_XOR)) + if (fgGlobalMorph && (varTypeIsIntegralOrI(tree->TypeGet())) && (tree->OperIs(GT_AND, GT_OR, GT_XOR))) { - tree = fgMorphOpCast(tree->AsOp()); + tree = fgMorphCastedBitwiseOp(tree->AsOp()); - // fgMorphOpCast may return a new tree + // fgMorphCastedBitwiseOp may return a new tree oper = tree->OperGet(); typ = tree->TypeGet(); - op1 = tree->AsOp()->gtOp1; - op2 = tree->AsOp()->gtOp2; + op1 = tree->AsOp()->gtGetOp1(); + op2 = tree->AsOp()->gtGetOp2(); } if (varTypeIsIntegralOrI(tree->TypeGet()) && tree->OperIs(GT_ADD, GT_MUL, GT_AND, GT_OR, GT_XOR)) From 838b35b8c109a56bba7ac810543c755eaebf9d2d Mon Sep 17 00:00:00 2001 From: Benjamin Hodgson Date: Mon, 6 Sep 2021 17:45:33 -0400 Subject: [PATCH 3/8] Don't call gtGetOp2 if tree was folded. If it was folded, it was folded to a unary (cast) operation and gtGetOp2() will crash. I also tweaked fgMorphCastedBitwiseOp to return nullptr if it didn't do anything (to match behaviour of fgMorphCommutative) --- src/coreclr/jit/morph.cpp | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index bce39b89079e40..f44c2860e22752 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11060,7 +11060,7 @@ GenTree* Compiler::fgMorphCommutative(GenTreeOp* tree) // tree - node to fold // // Return Value: -// A folded GenTree* instance, or the same GenTree* if it couldn't be folded +// A folded GenTree* instance, or nullptr if it couldn't be folded GenTree* Compiler::fgMorphCastedBitwiseOp(GenTreeOp* tree) { assert(varTypeIsIntegralOrI(tree)); @@ -11076,7 +11076,7 @@ GenTree* Compiler::fgMorphCastedBitwiseOp(GenTreeOp* tree) // bail if either operand is a checked cast if (op1->gtOverflowEx() || op2->gtOverflowEx()) { - return tree; + return nullptr; } var_types fromType = op1->AsCast()->CastOp()->TypeGet(); @@ -11085,7 +11085,7 @@ GenTree* Compiler::fgMorphCastedBitwiseOp(GenTreeOp* tree) if ((op2->CastFromType() != fromType) || (op2->CastToType() != toType) || (op2->IsUnsigned() != isUnsigned)) { - return tree; + return nullptr; } // Reuse gentree nodes: @@ -11113,7 +11113,7 @@ GenTree* Compiler::fgMorphCastedBitwiseOp(GenTreeOp* tree) return op1; } - return tree; + return nullptr; } /***************************************************************************** @@ -12802,13 +12802,16 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (fgGlobalMorph && (varTypeIsIntegralOrI(tree->TypeGet())) && (tree->OperIs(GT_AND, GT_OR, GT_XOR))) { - tree = fgMorphCastedBitwiseOp(tree->AsOp()); - - // fgMorphCastedBitwiseOp may return a new tree - oper = tree->OperGet(); - typ = tree->TypeGet(); - op1 = tree->AsOp()->gtGetOp1(); - op2 = tree->AsOp()->gtGetOp2(); + GenTree* result = fgMorphCastedBitwiseOp(tree->AsOp()); + if (result != nullptr) + { + // tree got folded to a unary (cast) op + tree = result; + oper = tree->OperGet(); + typ = tree->TypeGet(); + op1 = tree->AsOp()->gtGetOp1(); + op2 = nullptr; + } } if (varTypeIsIntegralOrI(tree->TypeGet()) && tree->OperIs(GT_ADD, GT_MUL, GT_AND, GT_OR, GT_XOR)) From acf28b84f510d2b7877ace40a6238cb6de9fa3fc Mon Sep 17 00:00:00 2001 From: Benjamin Hodgson Date: Mon, 6 Sep 2021 17:54:02 -0400 Subject: [PATCH 4/8] Code review changes for tests * Removed all but one csproj * Added tests for scenarios with overflow, compound exprs, side effects --- .../JIT/CodeGenBringUpTests/CastThenBinop.cs | 47 ++++++++++++++++++- ...enBinop_do.csproj => CastThenBinop.csproj} | 5 +- .../CastThenBinop_d.csproj | 13 ----- .../CastThenBinop_r.csproj | 13 ----- .../CastThenBinop_ro.csproj | 13 ----- 5 files changed, 47 insertions(+), 44 deletions(-) rename src/tests/JIT/CodeGenBringUpTests/{CastThenBinop_do.csproj => CastThenBinop.csproj} (66%) delete mode 100644 src/tests/JIT/CodeGenBringUpTests/CastThenBinop_d.csproj delete mode 100644 src/tests/JIT/CodeGenBringUpTests/CastThenBinop_r.csproj delete mode 100644 src/tests/JIT/CodeGenBringUpTests/CastThenBinop_ro.csproj diff --git a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop.cs b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop.cs index 2433094d28c24d..959f26114b786f 100644 --- a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop.cs +++ b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop.cs @@ -1,7 +1,7 @@ // 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.Runtime.CompilerServices; // Test for https://github.com/dotnet/runtime/issues/13816 @@ -19,6 +19,25 @@ static long UpcastAnd(int a, int b) return (long)a & (long)b; } + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + static long UpcastAnd_ComplexExpression(int a, int b) + { + return (long)(a - 2) & (long)(b + 1); + } + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + static long UpcastAnd_SideEffect(int a, int b, out int a1, out int b1) + { + return (long)(a1 = a) & (long)(b1 = b); + } + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + static int DowncastAnd_Overflow(int a, int b) + { + checked + { + return (byte)a & (byte)b; + } + } + public static int Main() { const int Pass = 100; @@ -33,6 +52,32 @@ public static int Main() return Fail; } + try + { + DowncastAnd_Overflow(0x100, 0xFF); + // should throw + return Fail; + } + catch (OverflowException) + { + // expected + } + + { + var result = UpcastAnd_ComplexExpression(0x0FF, 0xFF0); + if (result != 0xF1) + { + return Fail; + } + } + { + var result = UpcastAnd_SideEffect(0x0FF, 0xFF0, out var out1, out var out2); + if (result != 0xF0 || out1 != 0x0FF || out2 != 0xFF0) + { + return Fail; + } + } + return Pass; } } diff --git a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_do.csproj b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop.csproj similarity index 66% rename from src/tests/JIT/CodeGenBringUpTests/CastThenBinop_do.csproj rename to src/tests/JIT/CodeGenBringUpTests/CastThenBinop.csproj index 0df64817015e73..3d940585af3e04 100644 --- a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_do.csproj +++ b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop.csproj @@ -1,10 +1,7 @@ Exe - 1 - - - Full + None True diff --git a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_d.csproj b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_d.csproj deleted file mode 100644 index a6b32eb44172e5..00000000000000 --- a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_d.csproj +++ /dev/null @@ -1,13 +0,0 @@ - - - Exe - 1 - - - Full - False - - - - - diff --git a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_r.csproj b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_r.csproj deleted file mode 100644 index 2bd35b2343c0d5..00000000000000 --- a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_r.csproj +++ /dev/null @@ -1,13 +0,0 @@ - - - Exe - 1 - - - PdbOnly - False - - - - - diff --git a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_ro.csproj b/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_ro.csproj deleted file mode 100644 index 86fc858f449929..00000000000000 --- a/src/tests/JIT/CodeGenBringUpTests/CastThenBinop_ro.csproj +++ /dev/null @@ -1,13 +0,0 @@ - - - Exe - 1 - - - PdbOnly - True - - - - - From fa787fc4991aa4a0fd7f3dc68972a13d48caf197 Mon Sep 17 00:00:00 2001 From: Benjamin Hodgson Date: Mon, 6 Sep 2021 18:00:06 -0400 Subject: [PATCH 5/8] Add in some asserts and remove a redundant call --- src/coreclr/jit/morph.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index f44c2860e22752..03b0eb288050fc 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12800,11 +12800,13 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) op2 = tree->AsOp()->gtOp2; } - if (fgGlobalMorph && (varTypeIsIntegralOrI(tree->TypeGet())) && (tree->OperIs(GT_AND, GT_OR, GT_XOR))) + if (fgGlobalMorph && (varTypeIsIntegralOrI(tree)) && (tree->OperIs(GT_AND, GT_OR, GT_XOR))) { GenTree* result = fgMorphCastedBitwiseOp(tree->AsOp()); if (result != nullptr) { + assert(result->OperIs(GT_CAST)); + assert(result->gtOp2 == nullptr); // tree got folded to a unary (cast) op tree = result; oper = tree->OperGet(); From ae0250809eccb95da5e6ecf25fc9d66aeeeab0c1 Mon Sep 17 00:00:00 2001 From: Benjamin Hodgson Date: Mon, 6 Sep 2021 20:44:08 -0400 Subject: [PATCH 6/8] fix typo --- 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 03b0eb288050fc..33fc4aab09dbfb 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12806,7 +12806,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) if (result != nullptr) { assert(result->OperIs(GT_CAST)); - assert(result->gtOp2 == nullptr); + assert(result->AsOp()->gtOp2 == nullptr); // tree got folded to a unary (cast) op tree = result; oper = tree->OperGet(); From c32cb0fef910dc07b7eb21891f702b179a5c6a3a Mon Sep 17 00:00:00 2001 From: Benjamin Hodgson Date: Sat, 11 Sep 2021 10:33:56 -0400 Subject: [PATCH 7/8] Code review changes: * Formatting * Use getters instead of fields * set flags on op1 --- src/coreclr/jit/morph.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 33fc4aab09dbfb..c0343f8123348b 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11074,13 +11074,13 @@ GenTree* Compiler::fgMorphCastedBitwiseOp(GenTreeOp* tree) if (op1->OperIs(GT_CAST) && op2->OperIs(GT_CAST)) { // bail if either operand is a checked cast - if (op1->gtOverflowEx() || op2->gtOverflowEx()) + if (op1->gtOverflow() || op2->gtOverflow()) { return nullptr; } var_types fromType = op1->AsCast()->CastOp()->TypeGet(); - var_types toType = op1->AsCast()->gtCastType; + var_types toType = op1->AsCast()->CastToType(); bool isUnsigned = op1->IsUnsigned(); if ((op2->CastFromType() != fromType) || (op2->CastToType() != toType) || (op2->IsUnsigned() != isUnsigned)) @@ -11104,7 +11104,8 @@ GenTree* Compiler::fgMorphCastedBitwiseOp(GenTreeOp* tree) op1->gtType = genActualType(toType); op1->AsCast()->gtOp1 = tree; - op1->AsCast()->gtCastType = toType; + op1->AsCast()->CastToType() = toType; + op1->SetAllEffectsFlags(tree); // no need to update isUnsigned DEBUG_DESTROY_NODE(op2); @@ -12800,7 +12801,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) op2 = tree->AsOp()->gtOp2; } - if (fgGlobalMorph && (varTypeIsIntegralOrI(tree)) && (tree->OperIs(GT_AND, GT_OR, GT_XOR))) + if (fgGlobalMorph && varTypeIsIntegralOrI(tree) && tree->OperIs(GT_AND, GT_OR, GT_XOR)) { GenTree* result = fgMorphCastedBitwiseOp(tree->AsOp()); if (result != nullptr) From 0b599285e54b465c88453268df1d9d2d8630351e Mon Sep 17 00:00:00 2001 From: Benjamin Hodgson Date: Sat, 11 Sep 2021 16:19:28 -0400 Subject: [PATCH 8/8] Fix formatting --- src/coreclr/jit/morph.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index c0343f8123348b..45525ceca0ad11 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11066,8 +11066,8 @@ GenTree* Compiler::fgMorphCastedBitwiseOp(GenTreeOp* tree) assert(varTypeIsIntegralOrI(tree)); assert(tree->OperIs(GT_OR, GT_AND, GT_XOR)); - GenTree* op1 = tree->gtGetOp1(); - GenTree* op2 = tree->gtGetOp2(); + GenTree* op1 = tree->gtGetOp1(); + GenTree* op2 = tree->gtGetOp2(); genTreeOps oper = tree->OperGet(); // see whether both ops are casts, with matching to and from types. @@ -11079,9 +11079,9 @@ GenTree* Compiler::fgMorphCastedBitwiseOp(GenTreeOp* tree) return nullptr; } - var_types fromType = op1->AsCast()->CastOp()->TypeGet(); - var_types toType = op1->AsCast()->CastToType(); - bool isUnsigned = op1->IsUnsigned(); + var_types fromType = op1->AsCast()->CastOp()->TypeGet(); + var_types toType = op1->AsCast()->CastToType(); + bool isUnsigned = op1->IsUnsigned(); if ((op2->CastFromType() != fromType) || (op2->CastToType() != toType) || (op2->IsUnsigned() != isUnsigned)) { @@ -11098,12 +11098,12 @@ GenTree* Compiler::fgMorphCastedBitwiseOp(GenTreeOp* tree) // // (op2 becomes garbage) - tree->gtOp1 = op1->AsCast()->CastOp(); - tree->gtOp2 = op2->AsCast()->CastOp(); + tree->gtOp1 = op1->AsCast()->CastOp(); + tree->gtOp2 = op2->AsCast()->CastOp(); tree->gtType = fromType; - op1->gtType = genActualType(toType); - op1->AsCast()->gtOp1 = tree; + op1->gtType = genActualType(toType); + op1->AsCast()->gtOp1 = tree; op1->AsCast()->CastToType() = toType; op1->SetAllEffectsFlags(tree); // no need to update isUnsigned