From 1d4b0f697202e8e47347058fd87849cf7ea7480e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 11 Oct 2022 18:35:20 +0200 Subject: [PATCH 1/6] JIT: Fix profiler tail call insertion logic for FIELD_LIST This logic was not handling FIELD_LIST and was also not handling linear order appropriately. The logic is still a bit odd, it would probably be better to use the same kind of logic as CFG (moving PUTARG nodes ahead of the profiler hook instead), but in practice this seems to be ok. Fix #76879 --- src/coreclr/jit/lower.cpp | 111 +++++++++++++++++++++++++++++--------- src/coreclr/jit/lower.h | 2 + 2 files changed, 87 insertions(+), 26 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 5c09733806790..947b0f7a77d2f 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1711,45 +1711,104 @@ void Lowering::InsertProfTailCallHook(GenTreeCall* call, GenTree* insertionPoint if (insertionPoint == nullptr) { - for (CallArg& arg : call->gtArgs.EarlyArgs()) - { - assert(!arg.GetEarlyNode()->OperIs(GT_PUTARG_REG)); // We don't expect to see these in early args - - if (arg.GetEarlyNode()->OperIs(GT_PUTARG_STK)) - { - // found it - insertionPoint = arg.GetEarlyNode(); - break; - } - } + insertionPoint = FindEarliestPutArg(call); if (insertionPoint == nullptr) { - for (CallArg& arg : call->gtArgs.LateArgs()) - { - if (arg.GetLateNode()->OperIs(GT_PUTARG_REG, GT_PUTARG_STK)) - { - // found it - insertionPoint = arg.GetLateNode(); - break; - } - } - - // If there are no args, insert before the call node - if (insertionPoint == nullptr) - { - insertionPoint = call; - } + insertionPoint = call; } } #endif // !defined(TARGET_X86) assert(insertionPoint != nullptr); + JITDUMP("Inserting profiler tail call before [%06u]\n", comp->dspTreeID(insertionPoint)); + GenTree* profHookNode = new (comp, GT_PROF_HOOK) GenTree(GT_PROF_HOOK, TYP_VOID); BlockRange().InsertBefore(insertionPoint, profHookNode); } +//------------------------------------------------------------------------ +// FindEarliestPutArg: Find the earliest direct PUTARG operand of a call node in +// linear order. +// +// Arguments: +// call - the call +// +// Returns: +// A PUTARG_* node that is the earliest of the call, or nullptr if the call +// has no arguments. +// +GenTree* Lowering::FindEarliestPutArg(GenTreeCall* call) +{ + size_t numMarkedNodes = 0; + for (CallArg& arg : call->gtArgs.Args()) + { + GenTree* earlyNode = arg.GetEarlyNode(); + if (arg.GetEarlyNode() != nullptr) + { + numMarkedNodes += MarkPutArgNodes(arg.GetEarlyNode()); + } + + if (arg.GetLateNode() != nullptr) + { + numMarkedNodes += MarkPutArgNodes(arg.GetLateNode()); + } + } + + if (numMarkedNodes <= 0) + { + return nullptr; + } + + GenTree* node = call->gtPrev; + do + { + node = node->gtPrev; + + assert((node != nullptr) && "Reached beginning of basic block while looking for marked nodes"); + + if ((node->gtLIRFlags & LIR::Flags::Mark) != 0) + { + node->gtLIRFlags &= ~LIR::Flags::Mark; + numMarkedNodes--; + } + } while (numMarkedNodes > 0); + + return node; +} + +//------------------------------------------------------------------------ +// MarkPutArgNodes: Mark all direct operand PUTARG nodes with a LIR mark. +// +// Arguments: +// node - the node (either a field list or PUTARG node) +// +// Returns: +// The number of marks added. +// +size_t Lowering::MarkPutArgNodes(GenTree* node) +{ + assert(node->OperIsPutArg() || node->OperIsFieldList()); + + size_t result = 0; + if (node->OperIsFieldList()) + { + for (GenTreeFieldList::Use& operand : node->AsFieldList()->Uses()) + { + assert(operand.GetNode()->OperIsPutArg()); + result += MarkPutArgNodes(operand.GetNode()); + } + } + else + { + node->gtLIRFlags |= LIR::Flags::Mark; + result++; + } + + return result; +} + //------------------------------------------------------------------------ // LowerFastTailCall: Lower a call node dispatched as a fast tailcall (epilog + // jmp). diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index f37aa6d2be5e0..84ba314adc514 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -161,6 +161,8 @@ class Lowering final : public Phase GenTree* lookForUsesStart, GenTreeCall* callNode); void InsertProfTailCallHook(GenTreeCall* callNode, GenTree* insertionPoint); + GenTree* FindEarliestPutArg(GenTreeCall* call); + size_t MarkPutArgNodes(GenTree* node); GenTree* LowerVirtualVtableCall(GenTreeCall* call); GenTree* LowerVirtualStubCall(GenTreeCall* call); void LowerArgsForCall(GenTreeCall* call); From 3f0f79773833733218f3e11dd66696a051dc9a81 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 11 Oct 2022 18:39:15 +0200 Subject: [PATCH 2/6] Nit --- src/coreclr/jit/lower.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 947b0f7a77d2f..6112af80a8685 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1744,7 +1744,6 @@ GenTree* Lowering::FindEarliestPutArg(GenTreeCall* call) size_t numMarkedNodes = 0; for (CallArg& arg : call->gtArgs.Args()) { - GenTree* earlyNode = arg.GetEarlyNode(); if (arg.GetEarlyNode() != nullptr) { numMarkedNodes += MarkPutArgNodes(arg.GetEarlyNode()); From 278ed35243a8197604c93097295b9b5f60802047 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 11 Oct 2022 18:40:12 +0200 Subject: [PATCH 3/6] Nit --- src/coreclr/jit/lower.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 6112af80a8685..8c70f70bacf94 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1760,7 +1760,7 @@ GenTree* Lowering::FindEarliestPutArg(GenTreeCall* call) return nullptr; } - GenTree* node = call->gtPrev; + GenTree* node = call; do { node = node->gtPrev; From d8e881ac41404ab88b7178b7ed6566f15c37a94f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 11 Oct 2022 18:46:33 +0200 Subject: [PATCH 4/6] Add an assert --- src/coreclr/jit/lower.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 8c70f70bacf94..b0b4da8d5207e 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1774,6 +1774,7 @@ GenTree* Lowering::FindEarliestPutArg(GenTreeCall* call) } } while (numMarkedNodes > 0); + assert(node->OperIsPutArg()); return node; } From bd01df582430289ffa82ed2bc83ec0860966c68f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 11 Oct 2022 19:06:35 +0200 Subject: [PATCH 5/6] Remove illegal skip --- src/coreclr/jit/codegenarmarch.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index b50af8b21477c..9fa3560730719 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -3069,9 +3069,6 @@ void CodeGen::genCall(GenTreeCall* call) CallArgABIInformation& abiInfo = arg.AbiInfo; GenTree* argNode = arg.GetLateNode(); - // GT_RELOAD/GT_COPY use the child node - argNode = argNode->gtSkipReloadOrCopy(); - if (abiInfo.GetRegNum() == REG_STK) continue; From 8190ad5e79d49a700081504ac31ffe41ae8cf912 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 12 Oct 2022 17:59:25 +0200 Subject: [PATCH 6/6] Update lower.cpp --- src/coreclr/jit/lower.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index b0b4da8d5207e..7fef6d758e449 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1802,6 +1802,7 @@ size_t Lowering::MarkPutArgNodes(GenTree* node) } else { + assert((node->gtLIRFlags & LIR::Flags::Mark) == 0); node->gtLIRFlags |= LIR::Flags::Mark; result++; }