From f8fb84ca9ab13e1f81ad7ed5366c164bcf5bb1e2 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 19 Oct 2020 12:48:16 -0700 Subject: [PATCH] =?UTF-8?q?JIT:=20don't=20inline=20methods=20with=20small?= =?UTF-8?q?=20stackallocs=20if=20the=20call=20site=20is=20=E2=80=A6=20(#43?= =?UTF-8?q?516)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The logic in `fgInlinePrependStatements` that zero-initializes locals doesn't kick in for jit temps introduced when small stackallocs are optimized. So if we inline a method with a small stackalloc into a loop, the memory for the stackalloc doesn't get properly re-zeroed on each iteration. Fix by disallowing such inlines by adding an extra check: the call site must not be in a loop. Closes #43391. --- src/coreclr/src/jit/compiler.h | 1 + src/coreclr/src/jit/importer.cpp | 27 ++++++++++++++++--- .../JitBlue/Runtime_43391/Runtime_43391.cs | 26 ++++++++++++++++++ .../Runtime_43391/Runtime_43391.csproj | 14 ++++++++++ 4 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_43391/Runtime_43391.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_43391/Runtime_43391.csproj diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index b5d3249923e103..16b0386d5258d0 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -4182,6 +4182,7 @@ class Compiler BasicBlock* impPushCatchArgOnStack(BasicBlock* hndBlk, CORINFO_CLASS_HANDLE clsHnd, bool isSingleBlockFilter); + bool impBlockIsInALoop(BasicBlock* block); void impImportBlockCode(BasicBlock* block); void impReimportMarkBlock(BasicBlock* block); diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index 309a6fddeb3868..6e776576e374ee 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -10824,6 +10824,25 @@ GenTree* Compiler::impCastClassOrIsInstToTree(GenTree* op1, } while (0) #endif // DEBUG +//------------------------------------------------------------------------ +// impBlockIsInALoop: check if a block might be in a loop +// +// Arguments: +// block - block to check +// +// Returns: +// true if the block might be in a loop. +// +// Notes: +// Conservatively correct; may return true for some blocks that are +// not actually in loops. +// +bool Compiler::impBlockIsInALoop(BasicBlock* block) +{ + return (compIsForInlining() && ((impInlineInfo->iciBlock->bbFlags & BBF_BACKWARD_JUMP) != 0)) || + ((block->bbFlags & BBF_BACKWARD_JUMP) != 0); +} + #ifdef _PREFAST_ #pragma warning(push) #pragma warning(disable : 21000) // Suppress PREFast warning about overly large function @@ -14031,9 +14050,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) lvaSetStruct(lclNum, resolvedToken.hClass, true /* unsafe value cls check */); } - bool bbInALoop = - (compIsForInlining() && ((impInlineInfo->iciBlock->bbFlags & BBF_BACKWARD_JUMP) != 0)) || - ((block->bbFlags & BBF_BACKWARD_JUMP) != 0); + bool bbInALoop = impBlockIsInALoop(block); bool bbIsReturn = (block->bbJumpKind == BBJ_RETURN) && (!compIsForInlining() || (impInlineInfo->iciBlock->bbJumpKind == BBJ_RETURN)); LclVarDsc* const lclDsc = lvaGetDesc(lclNum); @@ -15146,6 +15163,8 @@ void Compiler::impImportBlockCode(BasicBlock* block) { const ssize_t allocSize = op2->AsIntCon()->IconValue(); + bool bbInALoop = impBlockIsInALoop(block); + if (allocSize == 0) { // Result is nullptr @@ -15153,7 +15172,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) op1 = gtNewIconNode(0, TYP_I_IMPL); convertedToLocal = true; } - else if ((allocSize > 0) && ((compCurBB->bbFlags & BBF_BACKWARD_JUMP) == 0)) + else if ((allocSize > 0) && !bbInALoop) { // Get the size threshold for local conversion ssize_t maxSize = DEFAULT_MAX_LOCALLOC_TO_LOCAL_SIZE; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_43391/Runtime_43391.cs b/src/tests/JIT/Regression/JitBlue/Runtime_43391/Runtime_43391.cs new file mode 100644 index 00000000000000..c5b849a72c3eb1 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_43391/Runtime_43391.cs @@ -0,0 +1,26 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Block inlining of small localloc callee if call site is in a loop. + +using System; + +class Runtime_43391 +{ + static int Main() + { + int r = 58; + for (int i = 1; i >= 0; i--) + { + r += Test(i); + } + return r; + } + + public static unsafe byte Test(int i) + { + byte* p = stackalloc byte[8]; + p[i] = 42; + return p[1]; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_43391/Runtime_43391.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_43391/Runtime_43391.csproj new file mode 100644 index 00000000000000..260db028de3d26 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_43391/Runtime_43391.csproj @@ -0,0 +1,14 @@ + + + Exe + 0 + + + true + None + True + + + + +