From 09ccc84a2b18f165f9aa005ea7b14f37ac94636a Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Mon, 30 Sep 2019 09:18:07 -0700 Subject: [PATCH 1/4] 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 Updated comments Rebased --- src/jit/optimizer.cpp | 49 ++++++++++++++++--- .../JitBlue/GitHub_26417/GitHub_26417.cs | 47 ++++++++++++++++++ .../JitBlue/GitHub_26417/GitHub_26417.csproj | 12 +++++ 3 files changed, 100 insertions(+), 8 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..88bd596d44c1 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) { + // 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; + } + } // For this purpose, 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. + // (So we don't use GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS here, since that includes exceptions.) + // if (tree->IsCall()) { // If it's a call, it must be a helper call that does not mutate the heap. @@ -7052,6 +7078,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 + + + + + From ea5ee5d016772b7f21d3589daf01fe75642e8212 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Fri, 4 Oct 2019 14:23:05 -0700 Subject: [PATCH 2/4] Update comment --- src/jit/optimizer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/jit/optimizer.cpp b/src/jit/optimizer.cpp index 88bd596d44c1..736d48f79415 100644 --- a/src/jit/optimizer.cpp +++ b/src/jit/optimizer.cpp @@ -7040,10 +7040,10 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo m_beforeSideEffect = false; } } - // For this purpose, we only care about memory side effects. We assume that expressions will + + // 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.) // if (tree->IsCall()) { From dc3f18a76876f8adbf43afab2013e0568c6c5a8d Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Fri, 4 Oct 2019 18:32:12 -0700 Subject: [PATCH 3/4] Added check for helper calls that throw, fixes Arm32 --- src/jit/optimizer.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/jit/optimizer.cpp b/src/jit/optimizer.cpp index 736d48f79415..b399dbffc1e9 100644 --- a/src/jit/optimizer.cpp +++ b/src/jit/optimizer.cpp @@ -7067,6 +7067,19 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo { m_beforeSideEffect = false; } + // Additional check for helper calls that thow 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)) From b953a48bd3a7c57061e75642f1ac2459577dbe47 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Fri, 4 Oct 2019 18:51:14 -0700 Subject: [PATCH 4/4] Jit format --- src/jit/optimizer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/jit/optimizer.cpp b/src/jit/optimizer.cpp index b399dbffc1e9..5c5644208217 100644 --- a/src/jit/optimizer.cpp +++ b/src/jit/optimizer.cpp @@ -7067,7 +7067,8 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo { m_beforeSideEffect = false; } - // Additional check for helper calls that thow exceptions + + // Additional check for helper calls that throw exceptions if (!treeIsInvariant) { // We have a tree that is not loop invariant and we thus cannot hoist @@ -7079,7 +7080,6 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo m_beforeSideEffect = false; } } - } } else if (tree->OperIs(GT_ASG))