Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fewer AddressExposedReason::TOO_CONSERVATIVE #59429

Merged
merged 6 commits into from
Sep 28, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/coreclr/jit/codegencommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7469,14 +7469,8 @@ void CodeGen::genFnProlog()

if (compiler->info.compPublishStubParam)
{
#if CPU_LOAD_STORE_ARCH
GetEmitter()->emitIns_S_R(ins_Store(TYP_I_IMPL), EA_PTRSIZE, REG_SECRET_STUB_PARAM,
compiler->lvaStubArgumentVar, 0);
#else
// mov [lvaStubArgumentVar], EAX
GetEmitter()->emitIns_AR_R(ins_Store(TYP_I_IMPL), EA_PTRSIZE, REG_SECRET_STUB_PARAM, genFramePointerReg(),
compiler->lvaTable[compiler->lvaStubArgumentVar].GetStackOffset());
#endif
assert(intRegState.rsCalleeRegArgMaskLiveIn & RBM_SECRET_STUB_PARAM);

// It's no longer live; clear it out so it can be used after this in the prolog
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4476,6 +4476,11 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
assert(lvaStubArgumentVar == BAD_VAR_NUM);
lvaStubArgumentVar = lvaGrabTempWithImplicitUse(false DEBUGARG("stub argument"));
lvaTable[lvaStubArgumentVar].lvType = TYP_I_IMPL;
// TODO-CQ: there is no need to mark it as doNotEnreg. There are no stores for this local
// before codegen so liveness and LSRA mark it as "liveIn" and always allocate a stack slot for it.
// However, it would be better to process it like other argument locals and keep it in
// a reg for the whole method without spilling to the stack when possible.
lvaSetVarDoNotEnregister(lvaStubArgumentVar DEBUGARG(DoNotEnregisterReason::VMNeedsStackAddr));
}
};
DoPhase(this, PHASE_PRE_IMPORT, preImportPhase);
Expand Down
5 changes: 0 additions & 5 deletions src/coreclr/jit/compiler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1758,11 +1758,6 @@ inline unsigned Compiler::lvaGrabTempWithImplicitUse(bool shortLifetime DEBUGARG

LclVarDsc* varDsc = &lvaTable[lclNum];

// This will prevent it from being optimized away
// TODO-CQ: We shouldn't have to go as far as to declare these
// address-exposed -- DoNotEnregister should suffice?
lvaSetVarAddrExposed(lclNum DEBUGARG(AddressExposedReason::TOO_CONSERVATIVE));

// Note the implicit use
varDsc->lvImplicitlyReferenced = 1;

Expand Down
7 changes: 5 additions & 2 deletions src/coreclr/jit/fginline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1404,8 +1404,11 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)
if (!getNeedsGSSecurityCookie() && InlineeCompiler->getNeedsGSSecurityCookie())
{
setNeedsGSSecurityCookie();
const unsigned dummy = lvaGrabTempWithImplicitUse(false DEBUGARG("GSCookie dummy for inlinee"));
lvaTable[dummy].lvType = TYP_INT;
const unsigned dummy = lvaGrabTempWithImplicitUse(false DEBUGARG("GSCookie dummy for inlinee"));
LclVarDsc* gsCookieDummy = lvaGetDesc(dummy);
gsCookieDummy->lvType = TYP_INT;
gsCookieDummy->lvIsTemp = true; // It is not alive at all, set the flag to prevent zero-init.
lvaSetVarDoNotEnregister(dummy DEBUGARG(DoNotEnregisterReason::VMNeedsStackAddr));
}

// If there is non-NULL return, replace the GT_CALL with its return value expression,
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/jit/flowgraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2757,6 +2757,9 @@ void Compiler::fgAddInternal()

lvaInlinedPInvokeFrameVar = lvaGrabTempWithImplicitUse(false DEBUGARG("Pinvoke FrameVar"));

// Lowering::InsertPInvokeMethodProlog will create a call with this local addr as an argument.
lvaSetVarAddrExposed(lvaInlinedPInvokeFrameVar DEBUGARG(AddressExposedReason::ESCAPE_ADDRESS));

LclVarDsc* varDsc = &lvaTable[lvaInlinedPInvokeFrameVar];
varDsc->lvType = TYP_BLK;
// Make room for the inlined frame.
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12196,7 +12196,6 @@ void Compiler::impImportBlockCode(BasicBlock* block)
/* The ARGLIST cookie is a hidden 'last' parameter, we have already
adjusted the arg count cos this is like fetching the last param */
assertImp(0 < numArgs);
assert(lvaTable[lvaVarargsHandleArg].IsAddressExposed());
lclNum = lvaVarargsHandleArg;
op1 = gtNewLclvNode(lclNum, TYP_I_IMPL DEBUGARG(opcodeOffs + sz + 1));
op1 = gtNewOperNode(GT_ADDR, TYP_BYREF, op1);
Expand Down
18 changes: 9 additions & 9 deletions src/coreclr/jit/lclvars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -339,8 +339,11 @@ void Compiler::lvaInitTypeRef()
{
// Ensure that there will be at least one stack variable since
// we require that the GSCookie does not have a 0 stack offset.
unsigned dummy = lvaGrabTempWithImplicitUse(false DEBUGARG("GSCookie dummy"));
lvaTable[dummy].lvType = TYP_INT;
unsigned dummy = lvaGrabTempWithImplicitUse(false DEBUGARG("GSCookie dummy"));
LclVarDsc* gsCookieDummy = lvaGetDesc(dummy);
gsCookieDummy->lvType = TYP_INT;
gsCookieDummy->lvIsTemp = true; // It is not alive at all, set the flag to prevent zero-init.
lvaSetVarDoNotEnregister(dummy DEBUGARG(DoNotEnregisterReason::VMNeedsStackAddr));
}

// Allocate the lvaOutgoingArgSpaceVar now because we can run into problems in the
Expand Down Expand Up @@ -1228,14 +1231,10 @@ void Compiler::lvaInitVarArgsHandle(InitVarDscInfo* varDscInfo)
LclVarDsc* varDsc = varDscInfo->varDsc;
varDsc->lvType = TYP_I_IMPL;
varDsc->lvIsParam = 1;
// Make sure this lives in the stack -- address may be reported to the VM.
// TODO-CQ: This should probably be:
// lvaSetVarDoNotEnregister(varDscInfo->varNum DEBUGARG(VMNeedsStackAddr));
// But that causes problems, so, for expedience, I switched back to this heavyweight
// hammer. But I think it should be possible to switch; it may just work now
// that other problems are fixed.
lvaSetVarAddrExposed(varDscInfo->varNum DEBUGARG(AddressExposedReason::TOO_CONSERVATIVE));
assert(mostRecentlyActivePhase == PHASE_PRE_IMPORT);

// TODO-Cleanup: this is preImportation phase, why do we try to work with regs here?
// Should it be just deleted?
if (varDscInfo->canEnreg(TYP_I_IMPL))
{
/* Another register argument */
Expand Down Expand Up @@ -4348,6 +4347,7 @@ void Compiler::lvaMarkLocalVars()
lvaPSPSym = lvaGrabTempWithImplicitUse(false DEBUGARG("PSPSym"));
LclVarDsc* lclPSPSym = &lvaTable[lvaPSPSym];
lclPSPSym->lvType = TYP_I_IMPL;
lvaSetVarDoNotEnregister(lvaPSPSym DEBUGARG(DoNotEnregisterReason::VMNeedsStackAddr));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now this is the main reason for VMNeedsStackAddr but if I understand correctly we actually report this to VM so we can't do anything about it.

}
#endif // FEATURE_EH_FUNCLETS

Expand Down
19 changes: 14 additions & 5 deletions src/coreclr/jit/lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4066,8 +4066,11 @@ void Lowering::InsertPInvokeMethodProlog()
const CORINFO_EE_INFO* pInfo = comp->eeGetEEInfo();
const CORINFO_EE_INFO::InlinedCallFrameInfo& callFrameInfo = pInfo->inlinedCallFrameInfo;

// First arg: &compiler->lvaInlinedPInvokeFrameVar + callFrameInfo.offsetOfFrameVptr

// First arg: &compiler->lvaInlinedPInvokeFrameVar + callFrameInfo.offsetOfFrameVptr
#if defined(DEBUG)
const LclVarDsc* inlinedPInvokeDsc = comp->lvaGetDesc(comp->lvaInlinedPInvokeFrameVar);
assert(inlinedPInvokeDsc->IsAddressExposed());
#endif // DEBUG
GenTree* frameAddr = new (comp, GT_LCL_FLD_ADDR)
GenTreeLclFld(GT_LCL_FLD_ADDR, TYP_BYREF, comp->lvaInlinedPInvokeFrameVar, callFrameInfo.offsetOfFrameVptr);

Expand Down Expand Up @@ -4111,7 +4114,7 @@ void Lowering::InsertPInvokeMethodProlog()
GenTreeLclFld(GT_STORE_LCL_FLD, TYP_I_IMPL, comp->lvaInlinedPInvokeFrameVar, callFrameInfo.offsetOfCallSiteSP);
storeSP->gtOp1 = PhysReg(REG_SPBASE);
storeSP->gtFlags |= GTF_VAR_DEF;
comp->lvaSetVarDoNotEnregister(comp->lvaInlinedPInvokeFrameVar DEBUGARG(DoNotEnregisterReason::LocalField));
assert(inlinedPInvokeDsc->lvDoNotEnregister);

firstBlockRange.InsertBefore(insertionPoint, LIR::SeqTree(comp, storeSP));
DISPTREERANGE(firstBlockRange, storeSP);
Expand All @@ -4127,6 +4130,8 @@ void Lowering::InsertPInvokeMethodProlog()
GenTreeLclFld* storeFP =
new (comp, GT_STORE_LCL_FLD) GenTreeLclFld(GT_STORE_LCL_FLD, TYP_I_IMPL, comp->lvaInlinedPInvokeFrameVar,
callFrameInfo.offsetOfCalleeSavedFP);
assert(inlinedPInvokeDsc->lvDoNotEnregister);

storeFP->gtOp1 = PhysReg(REG_FPBASE);
storeFP->gtFlags |= GTF_VAR_DEF;

Expand Down Expand Up @@ -4409,8 +4414,12 @@ void Lowering::InsertPInvokeCallEpilog(GenTreeCall* call)

// First argument is the address of the frame variable.
GenTree* frameAddr =
new (comp, GT_LCL_VAR) GenTreeLclVar(GT_LCL_VAR, TYP_BYREF, comp->lvaInlinedPInvokeFrameVar);
frameAddr->SetOperRaw(GT_LCL_VAR_ADDR);
new (comp, GT_LCL_FLD_ADDR) GenTreeLclFld(GT_LCL_FLD_ADDR, TYP_BYREF, comp->lvaInlinedPInvokeFrameVar, 0);

#if defined(DEBUG)
const LclVarDsc* inlinedPInvokeDsc = comp->lvaGetDesc(comp->lvaInlinedPInvokeFrameVar);
assert(inlinedPInvokeDsc->IsAddressExposed());
#endif // DEBUG

// Insert call to CORINFO_HELP_JIT_PINVOKE_END
GenTreeCall* helperCall =
Expand Down
7 changes: 2 additions & 5 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2098,7 +2098,8 @@ GenTree* Compiler::fgMakeTmpArgNode(fgArgTabEntry* curArgTabEntry)
{
var_types addrType = TYP_BYREF;
arg = gtNewOperNode(GT_ADDR, addrType, arg);
addrNode = arg;
lvaSetVarAddrExposed(tmpVarNum DEBUGARG(AddressExposedReason::ESCAPE_ADDRESS));
addrNode = arg;

#if FEATURE_MULTIREG_ARGS
#ifdef TARGET_ARM64
Expand Down Expand Up @@ -2137,10 +2138,6 @@ GenTree* Compiler::fgMakeTmpArgNode(fgArgTabEntry* curArgTabEntry)
if (addrNode != nullptr)
{
assert(addrNode->gtOper == GT_ADDR);

// This will prevent this LclVar from being optimized away
lvaSetVarAddrExposed(tmpVarNum DEBUGARG(AddressExposedReason::TOO_CONSERVATIVE));

// the child of a GT_ADDR is required to have this flag set
addrNode->AsOp()->gtOp1->gtFlags |= GTF_DONT_CSE;
}
Expand Down