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.

Sign up for GitHub

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

Fix GC hole on arm32 #50759

Merged
merged 1 commit into from
Apr 9, 2021
+51 −5
Merged

Fix GC hole on arm32 #50759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 30 additions & 2 deletions src/coreclr/jit/emitarm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7742,7 +7742,7 @@ void emitter::emitInsLoadStoreOp(instruction ins, emitAttr attr, regNumber dataR

if (addr->isContained())
{
assert(addr->OperGet() == GT_LCL_VAR_ADDR || addr->OperGet() == GT_LEA);
assert(addr->OperIs(GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR, GT_LEA));

DWORD lsl = 0;

Expand Down Expand Up @@ -7827,7 +7827,21 @@ void emitter::emitInsLoadStoreOp(instruction ins, emitAttr attr, regNumber dataR
}
else // no Index
{
if (emitIns_valid_imm_for_ldst_offset(offset, attr))
if (addr->OperIs(GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR))
{
GenTreeLclVarCommon* varNode = addr->AsLclVarCommon();
unsigned lclNum = varNode->GetLclNum();
unsigned offset = varNode->GetLclOffs();
if (emitInsIsStore(ins))
{
emitIns_S_R(ins, attr, dataReg, lclNum, offset);
}
else
{
emitIns_R_S(ins, attr, dataReg, lclNum, offset);
}
}
else if (emitIns_valid_imm_for_ldst_offset(offset, attr))
{
// Then load/store dataReg from/to [memBase + offset]
emitIns_R_R_I(ins, attr, dataReg, memBase->GetRegNum(), offset);
Expand All @@ -7847,6 +7861,20 @@ void emitter::emitInsLoadStoreOp(instruction ins, emitAttr attr, regNumber dataR
}
else
{
#ifdef DEBUG
if (addr->OperIs(GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR))
{
// If the local var is a gcref or byref, the local var better be untracked, because we have
// no logic here to track local variable lifetime changes, like we do in the contained case
// above. E.g., for a `str r0,[r1]` for byref `r1` to local `V01`, we won't store the local
// `V01` and so the emitter can't update the GC lifetime for `V01` if this is a variable birth.
GenTreeLclVarCommon* varNode = addr->AsLclVarCommon();
unsigned lclNum = varNode->GetLclNum();
LclVarDsc* varDsc = emitComp->lvaGetDesc(lclNum);
assert(!varDsc->lvTracked);
}
#endif // DEBUG

if (offset != 0)
{
assert(emitIns_valid_imm_for_add(offset, INS_FLAGS_DONT_CARE));
Expand Down
14 changes: 14 additions & 0 deletions src/coreclr/jit/emitarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13510,6 +13510,20 @@ void emitter::emitInsLoadStoreOp(instruction ins, emitAttr attr, regNumber dataR
}
else // addr is not contained, so we evaluate it into a register
{
#ifdef DEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe void Lowering::CheckNode(Compiler* compiler, GenTree* node) is a better place for such check:

  1. it is earlier;
  2. it is one place for all platforms

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm worried about moving the assert to CheckNode:

  • It's pretty far upstream. Am I guaranteed no other phases are going to introduce the bad pattern after that is run and before this?
  • I'd have to check for any IND-like thing that might end up getting to this code path, then looking at the IND's address. Might I miss something doing that? It feels perhaps like doing too much anticipating what downstream phases and codegen are actually going to do. E.g., it looks like for STOREIND(LCL_VAR_ADDR) of a SIMD12 type, the addr isn't contained. I'd have to check that.

Comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's pretty far upstream. Am I guaranteed no other phases are going to introduce the bad pattern after that is run and before this?

We are adding "tracked" before Lowering and "contained" during Lowering, so after Lowering both should be stable (there is one exception for of ClearContained for bitcast but it is not important here)

I'd have to check for any IND-like thing that might end up getting to this code path, then looking at the IND's address. Might I miss something doing that? It feels perhaps like doing too much anticipating what downstream phases and codegen are actually going to do. E.g., it looks like for STOREIND(LCL_VAR_ADDR) of a SIMD12 type, the addr isn't contained. I'd have to check that.

I was thinking about adding something like:

case GT_LCL_VAR_ADDR:
case GT_LCL_FLD_ADDR:
{
    const GenTreeLclVarCommon* varNode = tree->AsLclVarCommon();
    const LclVarDsc* varDsc  = emitComp->lvaGetDesc(varNode);
    assert(varNode->isContained() || !varDsc->lvTracked);
    break;
}

to Lowering::CheckNode.

would not it be enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't work. There are cases where the LCL_VAR_ADDR is a not required to be contained. I see such a case with STORE_OBJ(LCL_VAR_ADDR, LCL_VAR). I also see STOREIND-simd12(LCL_FLD_ADDR, LCLVAR-simd16).

If I add || !varTypeIsGC(varDsc) that's still not enough, as I see cases where we have not-contained tracked address loads, which is fine because genCodeForLclAddr does the right liveness updates for uses. The issue is for stores. So, I'd need to look at the context (parent node). And it's not clear that just STOREIND is necessary to check.

Bottom-line: does this change really need this more general assert in a common location? Or can I leave it as-is?

if (addr->OperIs(GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR))
{
// If the local var is a gcref or byref, the local var better be untracked, because we have
// no logic here to track local variable lifetime changes, like we do in the contained case
// above. E.g., for a `str r0,[r1]` for byref `r1` to local `V01`, we won't store the local
// `V01` and so the emitter can't update the GC lifetime for `V01` if this is a variable birth.
GenTreeLclVarCommon* varNode = addr->AsLclVarCommon();
unsigned lclNum = varNode->GetLclNum();
LclVarDsc* varDsc = emitComp->lvaGetDesc(lclNum);
assert(!varDsc->lvTracked);
}
#endif // DEBUG

// Then load/store dataReg from/to [addrReg]
emitIns_R_R(ins, attr, dataReg, addr->GetRegNum());
}
Expand Down
10 changes: 7 additions & 3 deletions src/coreclr/jit/lowerarmarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1453,13 +1453,17 @@ void Lowering::ContainCheckIndir(GenTreeIndir* indirNode)
MakeSrcContained(indirNode, addr);
}
}
else if (addr->OperIs(GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR))
{
// These nodes go into an addr mode:
// - GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR is a stack addr mode.
MakeSrcContained(indirNode, addr);
}
#ifdef TARGET_ARM64
else if (addr->OperIs(GT_CLS_VAR_ADDR, GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR))
else if (addr->OperIs(GT_CLS_VAR_ADDR))
{
// These nodes go into an addr mode:
// - GT_CLS_VAR_ADDR turns into a constant.
// - GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR is a stack addr mode.

// make this contained, it turns into a constant that goes into an addr mode
MakeSrcContained(indirNode, addr);
}
Expand Down