diff --git a/src/jit/optimizer.cpp b/src/jit/optimizer.cpp index 7f3bb2b1a915..60d180a43184 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: @@ -7015,41 +7027,55 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo // if (m_beforeSideEffect) { - // 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.) - if (tree->IsCall()) + // If the current tree isn't invariant (and thus prevents hoisting) then we have to check for any possible side effect + if (!treeIsInvariant) { - // If it's a call, it must be a helper call that does not mutate the heap. - // Further, if it may run a cctor, it must be labeled as "Hoistable" - // (meaning it won't run a cctor because the class is not precise-init). - GenTreeCall* call = tree->AsCall(); - if (call->gtCallType != CT_HELPER) + // Here we know that we have a tree that we are not goimg to hoist + // If it has any side effects, then we need to set m_beforeSideEffect to false. + if ((tree->gtFlags & GTF_EXCEPT) != 0) { m_beforeSideEffect = false; } - else + } + else // this tree is invariant and could still be hoisted + { + // 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.) + if (tree->IsCall()) { - CorInfoHelpFunc helpFunc = eeGetHelperNum(call->gtCallMethHnd); - if (s_helperCallProperties.MutatesHeap(helpFunc)) + // If it's a call, it must be a helper call that does not mutate the heap. + // Further, if it may run a cctor, it must be labeled as "Hoistable" + // (meaning it won't run a cctor because the class is not precise-init). + GenTreeCall* call = tree->AsCall(); + if (call->gtCallType != CT_HELPER) { m_beforeSideEffect = false; } - else if (s_helperCallProperties.MayRunCctor(helpFunc) && - (call->gtFlags & GTF_CALL_HOISTABLE) == 0) + else { - m_beforeSideEffect = false; + CorInfoHelpFunc helpFunc = eeGetHelperNum(call->gtCallMethHnd); + if (s_helperCallProperties.MutatesHeap(helpFunc)) + { + m_beforeSideEffect = false; + } + else if (s_helperCallProperties.MayRunCctor(helpFunc) && + (call->gtFlags & GTF_CALL_HOISTABLE) == 0) + { + m_beforeSideEffect = false; + } } } - } - else if (tree->OperIs(GT_ASG)) - { - // If the LHS of the assignment has a global reference, then assume it's a global side effect. - GenTree* lhs = tree->gtOp.gtOp1; - if (lhs->gtFlags & GTF_GLOB_REF) + else if (tree->OperIs(GT_ASG)) { - m_beforeSideEffect = false; + // If the LHS of the assignment has a global reference, then assume it's a global side effect. + GenTree* lhs = tree->gtOp.gtOp1; + if (lhs->gtFlags & GTF_GLOB_REF) + { + m_beforeSideEffect = false; + } } } } 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 + + + + +