From 87c865f2a22a7e754ed614c63ee8c8a9f15089d3 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 22 Feb 2022 20:14:55 -0800 Subject: [PATCH 1/2] JIT: OSR jitstress fixes; enable struct promotion Fix OSR jitstress failures by disabling STRESS_LCL_FLD for two cases: * for Tier0 methods with patchpoints (because patchpoint info generation does not produce the right offsets when locals are padded) * for OSR locals in OSR methods (because they live on the Tier0 frame and so can't have their storage altered. Enable struct promotion for OSR locals. In most cases this ends up being dependent promotion. Enabling independent promotion will take more work. --- src/coreclr/jit/lclvars.cpp | 95 ++++++++++++++++++++++++++----------- 1 file changed, 67 insertions(+), 28 deletions(-) diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 87d50a0fc82cf5..32f433c1e0aac2 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -1829,13 +1829,6 @@ bool Compiler::StructPromotionHelper::CanPromoteStructVar(unsigned lclNum) return false; } - // TODO-CQ: enable promotion for OSR locals - if (compiler->lvaIsOSRLocal(lclNum)) - { - JITDUMP(" struct promotion of V%02u is disabled because it is an OSR local\n", lclNum); - return false; - } - CORINFO_CLASS_HANDLE typeHnd = varDsc->GetStructHnd(); assert(typeHnd != NO_CLASS_HANDLE); @@ -5934,6 +5927,11 @@ int Compiler::lvaAssignVirtualFrameOffsetToArg(unsigned lclNum, for (unsigned i = 0; i < varDsc->lvFieldCnt; i++) { LclVarDsc* fieldVarDsc = lvaGetDesc(firstFieldNum + i); + + JITDUMP("Adjusting offset of dependent V%02u of arg V%02u: parent %u field %u net %u\n", lclNum, + firstFieldNum + i, varDsc->GetStackOffset(), fieldVarDsc->lvFldOffset, + varDsc->GetStackOffset() + fieldVarDsc->lvFldOffset); + fieldVarDsc->SetStackOffset(varDsc->GetStackOffset() + fieldVarDsc->lvFldOffset); } } @@ -6409,7 +6407,7 @@ void Compiler::lvaAssignVirtualFrameOffsetsToLocals() In other words, we will not calculate the "base" address of the struct local if the promotion type is PROMOTION_TYPE_FIELD_DEPENDENT. */ - if (!opts.IsOSR() && lvaIsFieldOfDependentlyPromotedStruct(varDsc)) + if (lvaIsFieldOfDependentlyPromotedStruct(varDsc)) { continue; } @@ -6436,21 +6434,34 @@ void Compiler::lvaAssignVirtualFrameOffsetsToLocals() // will refer to their memory homes. if (lvaIsOSRLocal(lclNum)) { - // TODO-CQ: enable struct promotion for OSR locals; when that - // happens, figure out how to properly refer to the original - // frame slots for the promoted fields. - assert(!varDsc->lvIsStructField); + if (varDsc->lvIsStructField) + { + const unsigned parentLclNum = varDsc->lvParentLcl; + const int parentOriginalOffset = info.compPatchpointInfo->Offset(parentLclNum); + const int offset = originalFrameStkOffs + parentOriginalOffset + varDsc->lvFldOffset; + + JITDUMP("---OSR--- V%02u (promoted field of V%02u; on tier0 frame) tier0 FP-rel offset %d tier0 " + "frame offset %d field offset %d new virt offset " + "%d\n", + lclNum, parentLclNum, parentOriginalOffset, originalFrameStkOffs, varDsc->lvFldOffset, + offset); - // Add frampointer-relative offset of this OSR live local in the original frame - // to the offset of original frame in our new frame. - int originalOffset = info.compPatchpointInfo->Offset(lclNum); - int offset = originalFrameStkOffs + originalOffset; + lvaTable[lclNum].SetStackOffset(offset); + } + else + { + // Add frampointer-relative offset of this OSR live local in the original frame + // to the offset of original frame in our new frame. + const int originalOffset = info.compPatchpointInfo->Offset(lclNum); + const int offset = originalFrameStkOffs + originalOffset; - JITDUMP("---OSR--- V%02u (on tier0 frame) tier0 FP-rel offset %d tier0 frame offset %d new virt offset " + JITDUMP( + "---OSR--- V%02u (on tier0 frame) tier0 FP-rel offset %d tier0 frame offset %d new virt offset " "%d\n", lclNum, originalOffset, originalFrameStkOffs, offset); - lvaTable[lclNum].SetStackOffset(offset); + lvaTable[lclNum].SetStackOffset(offset); + } continue; } @@ -7091,18 +7102,25 @@ void Compiler::lvaAssignFrameOffsetsToPromotedStructs() // This is not true for the System V systems since there is no // outgoing args space. Assign the dependently promoted fields properly. // - if (varDsc->lvIsStructField -#if !defined(UNIX_AMD64_ABI) && !defined(TARGET_ARM) && !defined(TARGET_X86) - // ARM: lo/hi parts of a promoted long arg need to be updated. + CLANG_FORMAT_COMMENT_ANCHOR; - // For System V platforms there is no outgoing args space. +#if defined(UNIX_AMD64_ABI) || defined(TARGET_ARM) || defined(TARGET_X86) + // ARM: lo/hi parts of a promoted long arg need to be updated. + // + // For System V platforms there is no outgoing args space. + // + // For System V and x86, a register passed struct arg is homed on the stack in a separate local var. + // The offset of these structs is already calculated in lvaAssignVirtualFrameOffsetToArg methos. + // Make sure the code below is not executed for these structs and the offset is not changed. + // + const bool mustProcessParams = true; +#else + // OSR must also assign offsts here. + // + const bool mustProcessParams = opts.IsOSR(); +#endif // defined(UNIX_AMD64_ABI) || defined(TARGET_ARM) || defined(TARGET_X86) - // For System V and x86, a register passed struct arg is homed on the stack in a separate local var. - // The offset of these structs is already calculated in lvaAssignVirtualFrameOffsetToArg methos. - // Make sure the code below is not executed for these structs and the offset is not changed. - && !varDsc->lvIsParam -#endif // !defined(UNIX_AMD64_ABI) && !defined(TARGET_ARM) && !defined(TARGET_X86) - ) + if (varDsc->lvIsStructField && (!varDsc->lvIsParam || mustProcessParams)) { LclVarDsc* parentvarDsc = lvaGetDesc(varDsc->lvParentLcl); lvaPromotionType promotionType = lvaGetPromotionType(parentvarDsc); @@ -7119,6 +7137,9 @@ void Compiler::lvaAssignFrameOffsetsToPromotedStructs() noway_assert(varDsc->lvOnFrame); if (parentvarDsc->lvOnFrame) { + JITDUMP("Adjusting offset of dependent V%02u of V%02u: parent %u field %u net %u\n", lclNum, + varDsc->lvParentLcl, parentvarDsc->GetStackOffset(), varDsc->lvFldOffset, + parentvarDsc->GetStackOffset() + varDsc->lvFldOffset); varDsc->SetStackOffset(parentvarDsc->GetStackOffset() + varDsc->lvFldOffset); } else @@ -7922,6 +7943,24 @@ Compiler::fgWalkResult Compiler::lvaStressLclFldCB(GenTree** pTree, fgWalkData* return WALK_SKIP_SUBTREES; } + // Ignore OSR locals; if in memory, they will live on the + // Tier0 frame and so can't have their storage adjusted. + // + if (pComp->lvaIsOSRLocal(lclNum)) + { + varDsc->lvNoLclFldStress = true; + return WALK_SKIP_SUBTREES; + } + + // Likewise for Tier0 methods with patchpoints -- + // if we modify them we'll misreport their locations in the patchpoint info. + // + if (pComp->doesMethodHavePatchpoints() || pComp->doesMethodHavePartialCompilationPatchpoints()) + { + varDsc->lvNoLclFldStress = true; + return WALK_SKIP_SUBTREES; + } + // Fix for lcl_fld stress mode if (varDsc->lvKeepType) { From 2b244cc939f3cd0b7dcfd057d66f04fabfcf1c26 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 25 Feb 2022 15:09:37 -0800 Subject: [PATCH 2/2] Fix typo Co-authored-by: Bruce Forstall --- src/coreclr/jit/lclvars.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 32f433c1e0aac2..a60eef7a5c3654 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -7115,7 +7115,7 @@ void Compiler::lvaAssignFrameOffsetsToPromotedStructs() // const bool mustProcessParams = true; #else - // OSR must also assign offsts here. + // OSR must also assign offsets here. // const bool mustProcessParams = opts.IsOSR(); #endif // defined(UNIX_AMD64_ABI) || defined(TARGET_ARM) || defined(TARGET_X86)