From 985f8ef146f6c19a7190acaa9c48e3334cb54616 Mon Sep 17 00:00:00 2001 From: Eugene Rozenfeld Date: Mon, 3 Aug 2020 14:59:27 -0700 Subject: [PATCH] Don't remove dead indirections that may throw. (#40226) Liveness incorrectly removed indirection rhs's of dead assignments. This change fixes that. Fixes #39823. --- src/coreclr/src/jit/liveness.cpp | 16 +--- src/coreclr/src/jit/lower.cpp | 85 +++++++++++-------- src/coreclr/src/jit/lower.h | 3 + .../JitBlue/GitHub_39823/GitHub_39823.cs | 39 +++++++++ .../JitBlue/GitHub_39823/GitHub_39823.csproj | 14 +++ 5 files changed, 108 insertions(+), 49 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/GitHub_39823/GitHub_39823.cs create mode 100644 src/tests/JIT/Regression/JitBlue/GitHub_39823/GitHub_39823.csproj diff --git a/src/coreclr/src/jit/liveness.cpp b/src/coreclr/src/jit/liveness.cpp index 504a6c0fc37..75cf6e42dda 100644 --- a/src/coreclr/src/jit/liveness.cpp +++ b/src/coreclr/src/jit/liveness.cpp @@ -1980,11 +1980,10 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR GenTree* data = store->OperIs(GT_STOREIND) ? store->AsStoreInd()->Data() : store->AsBlk()->Data(); data->SetUnusedValue(); + if (data->isIndir()) { - // This is a block assignment. An indirection of the rhs is not considered - // to happen until the assignment so mark it as non-faulting. - data->gtFlags |= GTF_IND_NONFAULTING; + Lowering::TransformUnusedIndirection(data->AsIndir(), this, block); } blockRange.Remove(store); @@ -2322,17 +2321,6 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree, } #endif // DEBUG // Extract the side effects - if (rhsNode->TypeGet() == TYP_STRUCT) - { - // This is a block assignment. An indirection of the rhs is not considered to - // happen until the assignment, so we will extract the side effects from only - // the address. - if (rhsNode->OperIsIndir()) - { - assert(rhsNode->OperGet() != GT_NULLCHECK); - rhsNode = rhsNode->AsIndir()->Addr(); - } - } gtExtractSideEffList(rhsNode, &sideEffList); } diff --git a/src/coreclr/src/jit/lower.cpp b/src/coreclr/src/jit/lower.cpp index cc9e2891e9a..150bfd2d38d 100644 --- a/src/coreclr/src/jit/lower.cpp +++ b/src/coreclr/src/jit/lower.cpp @@ -6450,41 +6450,7 @@ void Lowering::LowerIndir(GenTreeIndir* ind) if (ind->OperIs(GT_NULLCHECK) || ind->IsUnusedValue()) { - // A nullcheck is essentially the same as an indirection with no use. - // The difference lies in whether a target register must be allocated. - // On XARCH we can generate a compare with no target register as long as the addresss - // is not contained. - // On ARM64 we can generate a load to REG_ZR in all cases. - // However, on ARM we must always generate a load to a register. - // In the case where we require a target register, it is better to use GT_IND, since - // GT_NULLCHECK is a non-value node and would therefore require an internal register - // to use as the target. That is non-optimal because it will be modeled as conflicting - // with the source register(s). - // So, to summarize: - // - On ARM64, always use GT_NULLCHECK for a dead indirection. - // - On ARM, always use GT_IND. - // - On XARCH, use GT_IND if we have a contained address, and GT_NULLCHECK otherwise. - // In all cases, change the type to TYP_INT. - // - ind->gtType = TYP_INT; -#ifdef TARGET_ARM64 - bool useNullCheck = true; -#elif TARGET_ARM - bool useNullCheck = false; -#else // TARGET_XARCH - bool useNullCheck = !ind->Addr()->isContained(); -#endif // !TARGET_XARCH - - if (useNullCheck && ind->OperIs(GT_IND)) - { - ind->ChangeOper(GT_NULLCHECK); - ind->ClearUnusedValue(); - } - else if (!useNullCheck && ind->OperIs(GT_NULLCHECK)) - { - ind->ChangeOper(GT_IND); - ind->SetUnusedValue(); - } + TransformUnusedIndirection(ind, comp, m_block); } } else @@ -6496,6 +6462,55 @@ void Lowering::LowerIndir(GenTreeIndir* ind) } } +//------------------------------------------------------------------------ +// TransformUnusedIndirection: change the opcode and the type of the unused indirection. +// +// Arguments: +// ind - Indirection to transform. +// comp - Compiler instance. +// block - Basic block of the indirection. +// +void Lowering::TransformUnusedIndirection(GenTreeIndir* ind, Compiler* comp, BasicBlock* block) +{ + // A nullcheck is essentially the same as an indirection with no use. + // The difference lies in whether a target register must be allocated. + // On XARCH we can generate a compare with no target register as long as the addresss + // is not contained. + // On ARM64 we can generate a load to REG_ZR in all cases. + // However, on ARM we must always generate a load to a register. + // In the case where we require a target register, it is better to use GT_IND, since + // GT_NULLCHECK is a non-value node and would therefore require an internal register + // to use as the target. That is non-optimal because it will be modeled as conflicting + // with the source register(s). + // So, to summarize: + // - On ARM64, always use GT_NULLCHECK for a dead indirection. + // - On ARM, always use GT_IND. + // - On XARCH, use GT_IND if we have a contained address, and GT_NULLCHECK otherwise. + // In all cases, change the type to TYP_INT. + // + assert(ind->OperIs(GT_NULLCHECK, GT_IND)); + + ind->gtType = TYP_INT; +#ifdef TARGET_ARM64 + bool useNullCheck = true; +#elif TARGET_ARM + bool useNullCheck = false; +#else // TARGET_XARCH + bool useNullCheck = !ind->Addr()->isContained(); +#endif // !TARGET_XARCH + + if (useNullCheck && ind->OperIs(GT_IND)) + { + comp->gtChangeOperToNullCheck(ind, block); + ind->ClearUnusedValue(); + } + else if (!useNullCheck && ind->OperIs(GT_NULLCHECK)) + { + ind->ChangeOper(GT_IND); + ind->SetUnusedValue(); + } +} + //------------------------------------------------------------------------ // LowerBlockStoreCommon: a common logic to lower STORE_OBJ/BLK/DYN_BLK. // diff --git a/src/coreclr/src/jit/lower.h b/src/coreclr/src/jit/lower.h index 0c620aebeb0..135446c7e84 100644 --- a/src/coreclr/src/jit/lower.h +++ b/src/coreclr/src/jit/lower.h @@ -8,6 +8,7 @@ XX Lower XX XX XX XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX +XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX */ #ifndef _LOWER_H_ @@ -532,6 +533,8 @@ public: bool IsContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode, GenTree* node, bool* supportsRegOptional); #endif // FEATURE_HW_INTRINSICS + static void TransformUnusedIndirection(GenTreeIndir* ind, Compiler* comp, BasicBlock* block); + private: static bool NodesAreEquivalentLeaves(GenTree* candidate, GenTree* storeInd); diff --git a/src/tests/JIT/Regression/JitBlue/GitHub_39823/GitHub_39823.cs b/src/tests/JIT/Regression/JitBlue/GitHub_39823/GitHub_39823.cs new file mode 100644 index 00000000000..501641e9b3c --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/GitHub_39823/GitHub_39823.cs @@ -0,0 +1,39 @@ +// 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; +using System.Diagnostics; + +class Runtime_39823 +{ + struct IntsWrapped + { + public int i1; + public int i2; + public int i3; + public int i4; + }; + + [MethodImpl(MethodImplOptions.NoInlining)] + static unsafe int TestUnusedObjCopy(IntsWrapped* ps) + { + IntsWrapped s = *ps; + return 100; + } + + + public static unsafe int Main() + { + try + { + TestUnusedObjCopy((IntsWrapped*)0); + Debug.Assert(false, "unreachable"); + } + catch + { + return 100; + } + return -1; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/GitHub_39823/GitHub_39823.csproj b/src/tests/JIT/Regression/JitBlue/GitHub_39823/GitHub_39823.csproj new file mode 100644 index 00000000000..51c683dceab --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/GitHub_39823/GitHub_39823.csproj @@ -0,0 +1,14 @@ + + + Exe + 1 + + + Full + True + True + + + + + -- GitLab