From f754df13dc882bedf89f2af38a7416247e6e6909 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Sat, 5 Oct 2019 17:39:37 -0700 Subject: [PATCH] New fix for #26417 - Don't allow the hoisting of GT_CLS_VARs that were assigned a constant value. (#26952) * Fix issue #26417 = Incorrect caching of loop variable Contributes to issue #7147 - JIT: Loop hoisting re-ordering exceptions Added the Test case for Issue 26417 --- src/jit/optimizer.cpp | 64 ++++++++++++++++--- .../JitBlue/GitHub_26417/GitHub_26417.cs | 47 ++++++++++++++ .../JitBlue/GitHub_26417/GitHub_26417.csproj | 12 ++++ 3 files changed, 114 insertions(+), 9 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.cs create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.csproj diff --git a/src/jit/optimizer.cpp b/src/jit/optimizer.cpp index 7f3bb2b1a915..5c5644208217 100644 --- a/src/jit/optimizer.cpp +++ b/src/jit/optimizer.cpp @@ -6795,8 +6795,20 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo bool IsTreeVNInvariant(GenTree* tree) { - return m_compiler->optVNIsLoopInvariant(tree->gtVNPair.GetLiberal(), m_loopNum, - &m_hoistContext->m_curLoopVnInvariantCache); + ValueNum vn = tree->gtVNPair.GetLiberal(); + if (m_compiler->vnStore->IsVNConstant(vn)) + { + // It is unsafe to allow a GT_CLS_VAR that has been assigned a constant. + // The logic in optVNIsLoopInvariant would consider it to be loop-invariant, even + // if the assignment of the constant to the GT_CLS_VAR was inside the loop. + // + if (tree->OperIs(GT_CLS_VAR)) + { + return false; + } + } + + return m_compiler->optVNIsLoopInvariant(vn, m_loopNum, &m_hoistContext->m_curLoopVnInvariantCache); } public: @@ -7003,22 +7015,36 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo // Is the value of the whole tree loop invariant? if (!treeIsInvariant) { + // Here we have a tree that is not loop invariant and we thus cannot hoist treeIsHoistable = false; } } - // Check if we need to set 'm_beforeSideEffect' to false. - // If we encounter a tree with a call in it - // or if we see an assignment to global we set it to false. + // Next check if we need to set 'm_beforeSideEffect' to false. // - // If we are already set to false then we can skip these checks + // If we have already set it to false then we can skip these checks // if (m_beforeSideEffect) { - // For this purpose, we only care about memory side effects. We assume that expressions will + // Is the value of the whole tree loop invariant? + if (!treeIsInvariant) + { + // We have a tree that is not loop invariant and we thus cannot hoist + assert(treeIsHoistable == false); + + // Check if we should clear m_beforeSideEffect. + // If 'tree' can throw an exception then we need to set m_beforeSideEffect to false. + // Note that calls are handled below + if (tree->OperMayThrow(m_compiler) && !tree->IsCall()) + { + m_beforeSideEffect = false; + } + } + + // In the section below, we only care about memory side effects. We assume that expressions will // be hoisted so that they are evaluated in the same order as they would have been in the loop, - // and therefore throw exceptions in the same order. (So we don't use GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS - // here, since that includes exceptions.) + // and therefore throw exceptions in the same order. + // if (tree->IsCall()) { // If it's a call, it must be a helper call that does not mutate the heap. @@ -7041,6 +7067,19 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo { m_beforeSideEffect = false; } + + // Additional check for helper calls that throw exceptions + if (!treeIsInvariant) + { + // We have a tree that is not loop invariant and we thus cannot hoist + assert(treeIsHoistable == false); + + // Does this helper call throw? + if (!s_helperCallProperties.NoThrow(helpFunc)) + { + m_beforeSideEffect = false; + } + } } } else if (tree->OperIs(GT_ASG)) @@ -7052,6 +7091,13 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo m_beforeSideEffect = false; } } + else if (tree->OperIs(GT_XADD, GT_XCHG, GT_LOCKADD, GT_CMPXCHG, GT_MEMORYBARRIER)) + { + // If this node is a MEMORYBARRIER or an Atomic operation + // then don't hoist and stop any further hoisting after this node + treeIsHoistable = false; + m_beforeSideEffect = false; + } } // If this 'tree' is hoistable then we return and the caller will diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.cs b/tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.cs new file mode 100644 index 000000000000..6871962398e4 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.cs @@ -0,0 +1,47 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.CompilerServices; + +class GitHub_26417 +{ + static int _a; + + [MethodImplAttribute(MethodImplOptions.NoInlining)] + static void MyWriteLine(int v) + { + Console.WriteLine(v); + if (v == 0) + { + throw new Exception(); + } + } + + [MethodImplAttribute(MethodImplOptions.NoInlining)] + static void Test() + { + _a = 1; + + while (_a == 1) + { + MyWriteLine(_a); + _a = 0; + } + } + + static int Main() + { + int result = 100; + try { + Test(); + } + catch (Exception) + { + Console.WriteLine("FAILED"); + result = -1; + } + return result; + } +} diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.csproj new file mode 100644 index 000000000000..f3e1cbd44b40 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + +