From 22c0a457c788af6315f2abea5d97f319cb90bb6c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 10 Aug 2024 08:29:22 -0700 Subject: [PATCH 1/4] JIT: limit phi-refinement to loop-invariant VNs PhiArg VNs should be invariant in the loop that contains the Phi, otherwise the same VN may refer to values from more than one iteration. Fixes #105792. --- src/coreclr/jit/valuenum.cpp | 55 ++++++++++++++++--- .../JitBlue/Runtime_105792/Runtime_105792.cs | 31 +++++++++++ .../Runtime_105792/Runtime_105792.csproj | 11 ++++ 3 files changed, 89 insertions(+), 8 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.csproj diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index fdddcaff70cd52..3c80348d752987 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -11161,16 +11161,55 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo ValueNumPair phiArgVNP = lvaGetDesc(phiArg)->GetPerSsaData(phiArg->GetSsaNum())->m_vnPair; -#ifdef DEBUG - if (verbose && isUpdate && (phiArgVNP != phiArg->gtVNPair)) + if (isUpdate && (phiArgVNP != phiArg->gtVNPair)) { - printf("Updating phi arg [%06u] VN from ", dspTreeID(phiArg)); - vnpPrint(phiArg->gtVNPair, 0); - printf(" to "); - vnpPrint(phiArgVNP, 0); - printf("\n"); - } + bool canUseNewVN = false; + + // We can potentially refine this phi arg. + // Make sure the new phi arg VN is loop invariant. + // + FlowGraphNaturalLoop* const vnLoop = vnStore->LoopOfVN(phiArgVNP.GetConservative()); + + if (vnLoop != nullptr) + { + FlowGraphNaturalLoop* const blockLoop = m_loops->GetLoopByHeader(blk); + assert(blockLoop != nullptr); + canUseNewVN = !blockLoop->ContainsLoop(vnLoop); + + if (!canUseNewVN) + { + JITDUMP("Can't refine [%06u] with " FMT_VN " -- varies in " FMT_LP ", contained in " FMT_LP "\n", + dspTreeID(phiArg), phiArgVNP.GetConservative(), vnLoop->GetIndex(), blockLoop->GetIndex()); + } + } + else + { + // phiArgVNP is invariant in all loops + // + canUseNewVN = true; + } + + if (canUseNewVN) + { + +#ifdef DEBUG + if (verbose) + { + printf("Updating phi arg [%06u] VN from ", dspTreeID(phiArg)); + vnpPrint(phiArg->gtVNPair, 0); + printf(" to "); + vnpPrint(phiArgVNP, 0); + printf("\n"); + } #endif + } + else + { + // Code below uses phiArgVNP, reset to the old value + // + phiArgVNP = phiArg->gtVNPair; + } + } phiArg->gtVNPair = phiArgVNP; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.cs b/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.cs new file mode 100644 index 00000000000000..8952244655dba4 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.cs @@ -0,0 +1,31 @@ +// 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 Xunit; + +public class Runtime_105792 +{ + [MethodImpl(MethodImplOptions.NoInlining)] + static int Problem(int x) + { + int y = 0; + while (x != 0) + { + if (y == x) return -1; + y = x; + Update(ref x); + } + return x; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Update(ref int x) + { + x = x - 1; + } + + [Fact] + public static int Test() => Problem(10) + 100; +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.csproj new file mode 100644 index 00000000000000..efa9e9b022442a --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.csproj @@ -0,0 +1,11 @@ + + + True + + + + + + + + From e011f099d8848656ace726f352fe4a10178253ae Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 10 Aug 2024 17:56:05 -0700 Subject: [PATCH 2/4] review feedback --- src/coreclr/jit/valuenum.cpp | 32 ++++--------------- .../JitBlue/Runtime_105792/Runtime_105792.cs | 20 ++++++++++-- .../Runtime_105792/Runtime_105792.csproj | 3 -- 3 files changed, 24 insertions(+), 31 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 3c80348d752987..81ef75fe6a3066 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -11141,6 +11141,7 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo GenTreePhi* phiNode = newSsaDef->AsLclVar()->Data()->AsPhi(); ValueNumPair sameVNP; + VNSet loopInvariantCache(getAllocator(CMK_ValueNumber)); for (GenTreePhi::Use& use : phiNode->Uses()) { @@ -11163,35 +11164,11 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo if (isUpdate && (phiArgVNP != phiArg->gtVNPair)) { - bool canUseNewVN = false; - - // We can potentially refine this phi arg. - // Make sure the new phi arg VN is loop invariant. - // - FlowGraphNaturalLoop* const vnLoop = vnStore->LoopOfVN(phiArgVNP.GetConservative()); - - if (vnLoop != nullptr) - { - FlowGraphNaturalLoop* const blockLoop = m_loops->GetLoopByHeader(blk); - assert(blockLoop != nullptr); - canUseNewVN = !blockLoop->ContainsLoop(vnLoop); - - if (!canUseNewVN) - { - JITDUMP("Can't refine [%06u] with " FMT_VN " -- varies in " FMT_LP ", contained in " FMT_LP "\n", - dspTreeID(phiArg), phiArgVNP.GetConservative(), vnLoop->GetIndex(), blockLoop->GetIndex()); - } - } - else - { - // phiArgVNP is invariant in all loops - // - canUseNewVN = true; - } + FlowGraphNaturalLoop* const blockLoop = m_loops->GetLoopByHeader(blk); + bool const canUseNewVN = optVNIsLoopInvariant(phiArgVNP.GetConservative(), blockLoop, &loopInvariantCache); if (canUseNewVN) { - #ifdef DEBUG if (verbose) { @@ -11205,6 +11182,9 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo } else { + JITDUMP("Can't update phi arg [%06u] with " FMT_VN " -- varies in " FMT_LP "\n", dspTreeID(phiArg), + phiArgVNP.GetConservative(), blockLoop->GetIndex()); + // Code below uses phiArgVNP, reset to the old value // phiArgVNP = phiArg->gtVNPair; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.cs b/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.cs index 8952244655dba4..993905d864e2d8 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.cs @@ -8,7 +8,7 @@ public class Runtime_105792 { [MethodImpl(MethodImplOptions.NoInlining)] - static int Problem(int x) + static int Problem1(int x) { int y = 0; while (x != 0) @@ -20,6 +20,19 @@ static int Problem(int x) return x; } + [MethodImpl(MethodImplOptions.NoInlining)] + static int Problem2(int x) + { + int y = 0; + while (x != 0) + { + if (y == x + 1) return -1; + y = x + 1; + Update(ref x); + } + return x; + } + [MethodImpl(MethodImplOptions.NoInlining)] static void Update(ref int x) { @@ -27,5 +40,8 @@ static void Update(ref int x) } [Fact] - public static int Test() => Problem(10) + 100; + public static int Test1() => Problem1(10) + 100; + + [Fact] + public static int Test2() => Problem2(10) + 100; } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.csproj index efa9e9b022442a..de6d5e08882e86 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.csproj +++ b/src/tests/JIT/Regression/JitBlue/Runtime_105792/Runtime_105792.csproj @@ -5,7 +5,4 @@ - - - From 72f2097f188e3e7f5c6b40073d1c2d912473fc69 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 12 Aug 2024 07:34:01 -0700 Subject: [PATCH 3/4] more aggressive version that allows PhiDefs --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/optimizer.cpp | 22 +++++++++++++++++++--- src/coreclr/jit/valuenum.cpp | 3 ++- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index b748f3b81138a3..8b9e1148e51921 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6999,7 +6999,7 @@ class Compiler // Returns true iff the ValueNum "vn" represents a value that is loop-invariant in "loop". // Constants and init values are always loop invariant. // VNPhi's connect VN's to the SSA definition, so we can know if the SSA def occurs in the loop. - bool optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNSet* recordedVNs); + bool optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNSet* recordedVNs, bool ignorePhiDefs = false); // Records the set of "side effects" of all loops: fields (object instance and static) // written to, and SZ-array element type equivalence classes updated. diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 816862ec091be3..f202656af8c245 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5147,7 +5147,23 @@ void Compiler::optHoistCandidate(GenTree* tree, Metrics.HoistedExpressions++; } -bool Compiler::optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNSet* loopVnInvariantCache) +//------------------------------------------------------------------------------ +// optVNIsLoopInvariant: Check if the value(s) this VN refers to may vary +// across iterations of a loop +// +// Arguments: +// vn -- the vn of interest +// loop -- the loop +// loopVnInvariantCache -- cache of prior results for this loop +// ignorePhiDefs -- if true, only consider memory dependence +// +// Returns: +// true if this VN represents a loop invariant +// +bool Compiler::optVNIsLoopInvariant(ValueNum vn, + FlowGraphNaturalLoop* loop, + VNSet* loopVnInvariantCache, + bool ignorePhiDefs) { // If it is not a VN, is not loop-invariant. if (vn == ValueNumStore::NoVN) @@ -5226,7 +5242,7 @@ bool Compiler::optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNS // TODO-CQ: We need to either make sure that *all* VN functions // always take VN args, or else have a list of arg positions to exempt, as implicitly // constant. - if (!optVNIsLoopInvariant(funcApp.m_args[i], loop, loopVnInvariantCache)) + if (!optVNIsLoopInvariant(funcApp.m_args[i], loop, loopVnInvariantCache, ignorePhiDefs)) { res = false; break; @@ -5238,7 +5254,7 @@ bool Compiler::optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNS { // Is the definition within the loop? If so, is not loop-invariant. LclSsaVarDsc* ssaDef = lvaTable[phiDef.LclNum].GetPerSsaData(phiDef.SsaDef); - res = !loop->ContainsBlock(ssaDef->GetBlock()); + res = ignorePhiDefs || !loop->ContainsBlock(ssaDef->GetBlock()); } else if (vnStore->GetMemoryPhiDef(vn, &memoryPhiDef)) { diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 81ef75fe6a3066..3e555810891a08 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -11165,7 +11165,8 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo if (isUpdate && (phiArgVNP != phiArg->gtVNPair)) { FlowGraphNaturalLoop* const blockLoop = m_loops->GetLoopByHeader(blk); - bool const canUseNewVN = optVNIsLoopInvariant(phiArgVNP.GetConservative(), blockLoop, &loopInvariantCache); + bool const canUseNewVN = optVNIsLoopInvariant(phiArgVNP.GetConservative(), blockLoop, &loopInvariantCache, + /* ignorePhiDefs */ true); if (canUseNewVN) { From ade605faa608b6243217628de5aebb18bb9eb512 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 12 Aug 2024 11:40:51 -0700 Subject: [PATCH 4/4] Revert "more aggressive version that allows PhiDefs" This reverts commit 72f2097f188e3e7f5c6b40073d1c2d912473fc69. --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/optimizer.cpp | 22 +++------------------- src/coreclr/jit/valuenum.cpp | 3 +-- 3 files changed, 5 insertions(+), 22 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 8b9e1148e51921..b748f3b81138a3 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -6999,7 +6999,7 @@ class Compiler // Returns true iff the ValueNum "vn" represents a value that is loop-invariant in "loop". // Constants and init values are always loop invariant. // VNPhi's connect VN's to the SSA definition, so we can know if the SSA def occurs in the loop. - bool optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNSet* recordedVNs, bool ignorePhiDefs = false); + bool optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNSet* recordedVNs); // Records the set of "side effects" of all loops: fields (object instance and static) // written to, and SZ-array element type equivalence classes updated. diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index f202656af8c245..816862ec091be3 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5147,23 +5147,7 @@ void Compiler::optHoistCandidate(GenTree* tree, Metrics.HoistedExpressions++; } -//------------------------------------------------------------------------------ -// optVNIsLoopInvariant: Check if the value(s) this VN refers to may vary -// across iterations of a loop -// -// Arguments: -// vn -- the vn of interest -// loop -- the loop -// loopVnInvariantCache -- cache of prior results for this loop -// ignorePhiDefs -- if true, only consider memory dependence -// -// Returns: -// true if this VN represents a loop invariant -// -bool Compiler::optVNIsLoopInvariant(ValueNum vn, - FlowGraphNaturalLoop* loop, - VNSet* loopVnInvariantCache, - bool ignorePhiDefs) +bool Compiler::optVNIsLoopInvariant(ValueNum vn, FlowGraphNaturalLoop* loop, VNSet* loopVnInvariantCache) { // If it is not a VN, is not loop-invariant. if (vn == ValueNumStore::NoVN) @@ -5242,7 +5226,7 @@ bool Compiler::optVNIsLoopInvariant(ValueNum vn, // TODO-CQ: We need to either make sure that *all* VN functions // always take VN args, or else have a list of arg positions to exempt, as implicitly // constant. - if (!optVNIsLoopInvariant(funcApp.m_args[i], loop, loopVnInvariantCache, ignorePhiDefs)) + if (!optVNIsLoopInvariant(funcApp.m_args[i], loop, loopVnInvariantCache)) { res = false; break; @@ -5254,7 +5238,7 @@ bool Compiler::optVNIsLoopInvariant(ValueNum vn, { // Is the definition within the loop? If so, is not loop-invariant. LclSsaVarDsc* ssaDef = lvaTable[phiDef.LclNum].GetPerSsaData(phiDef.SsaDef); - res = ignorePhiDefs || !loop->ContainsBlock(ssaDef->GetBlock()); + res = !loop->ContainsBlock(ssaDef->GetBlock()); } else if (vnStore->GetMemoryPhiDef(vn, &memoryPhiDef)) { diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 3e555810891a08..81ef75fe6a3066 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -11165,8 +11165,7 @@ void Compiler::fgValueNumberPhiDef(GenTreeLclVar* newSsaDef, BasicBlock* blk, bo if (isUpdate && (phiArgVNP != phiArg->gtVNPair)) { FlowGraphNaturalLoop* const blockLoop = m_loops->GetLoopByHeader(blk); - bool const canUseNewVN = optVNIsLoopInvariant(phiArgVNP.GetConservative(), blockLoop, &loopInvariantCache, - /* ignorePhiDefs */ true); + bool const canUseNewVN = optVNIsLoopInvariant(phiArgVNP.GetConservative(), blockLoop, &loopInvariantCache); if (canUseNewVN) {