Skip to content

Commit

Permalink
JIT: Add ordering side effects in fgMorphExpandInstanceField (#92998)
Browse files Browse the repository at this point in the history
In #92710 I removed an ordering side effect set on a LCL_VAR node in
fgMorphExpandInstanceField, since the side effect is meaningless on a
local. However, I did not realize that setting the ordering effect was
important because propagating it to parent nodes was actually necessary.

This PR explicitly sets the ordering effect on the ADD<byref> nodes
created, since forming these and reporting them to GC is illegal if the
base is invalid.

Additionally, it sets the ordering effect on the consuming indirection;
if that indirection made use of the fact that the FIELD_ADDR is non-null
to become non-faulting, then it has an ordering dependency once we
expand the FIELD_ADDR to a control-flow based null-check. This is not
actually necessary since we always set end up with an ADD<byref> that
propagates it, but that seems more like a coincidence than anything
else.

Fix #92990
  • Loading branch information
jakobbotsch authored Oct 5, 2023
1 parent 686bd7a commit ade2459
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 12 deletions.
4 changes: 2 additions & 2 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -5917,8 +5917,8 @@ class Compiler
// small; hence the other fields of MorphAddrContext.
struct MorphAddrContext
{
size_t m_totalOffset = 0; // Sum of offsets between the top-level indirection and here (current context).
bool m_used = false; // Whether this context was used to elide a null check.
GenTreeIndir* m_user = nullptr; // Indirection using this address.
size_t m_totalOffset = 0; // Sum of offsets between the top-level indirection and here (current context).
};

#ifdef FEATURE_SIMD
Expand Down
43 changes: 33 additions & 10 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5088,9 +5088,24 @@ GenTree* Compiler::fgMorphExpandInstanceField(GenTree* tree, MorphAddrContext* m
// will implicitly null-check the produced address.
addExplicitNullCheck = (mac == nullptr) || fgIsBigOffset(mac->m_totalOffset + fieldOffset);

if (!addExplicitNullCheck)
// The transformation here turns a value dependency (FIELD_ADDR being a
// known non-null operand) into a control-flow dependency (introducing
// explicit COMMA(NULLCHECK, ...)). This effectively "disconnects" the
// null check from the parent of the FIELD_ADDR node. For the cases
// where we made use of non-nullness we need to make the dependency
// explicit now.
if (addExplicitNullCheck)
{
mac->m_used = true;
if (mac != nullptr)
{
mac->m_user->SetHasOrderingSideEffect();
}
}
else
{
// We can elide the null check only by letting it happen as part of
// the consuming indirection, so it is no longer non-faulting.
mac->m_user->gtFlags &= ~GTF_IND_NONFAULTING;
}
}

Expand Down Expand Up @@ -5153,6 +5168,13 @@ GenTree* Compiler::fgMorphExpandInstanceField(GenTree* tree, MorphAddrContext* m
}

addr = gtNewOperNode(GT_ADD, (objRefType == TYP_I_IMPL) ? TYP_I_IMPL : TYP_BYREF, addr, offsetNode);

// We cannot form and GC report an invalid byref, so this must preserve
// its ordering with the null check.
if (addExplicitNullCheck && addr->TypeIs(TYP_BYREF))
{
addr->SetHasOrderingSideEffect();
}
}
#endif

Expand All @@ -5168,6 +5190,13 @@ GenTree* Compiler::fgMorphExpandInstanceField(GenTree* tree, MorphAddrContext* m
{
addr = gtNewOperNode(GT_ADD, (objRefType == TYP_I_IMPL) ? TYP_I_IMPL : TYP_BYREF, addr,
gtNewIconNode(fieldOffset, fieldSeq));

// We cannot form and GC report an invalid byref, so this must preserve
// its ordering with the null check.
if (addExplicitNullCheck && addr->TypeIs(TYP_BYREF))
{
addr->SetHasOrderingSideEffect();
}
}

if (addExplicitNullCheck)
Expand Down Expand Up @@ -8937,7 +8966,8 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
if (tree->OperIsIndir()) // TODO-CQ: add more operators here (e. g. atomics).
{
// Communicate to FIELD_ADDR morphing that the parent is an indirection.
mac = &indMac;
indMac.m_user = tree->AsIndir();
mac = &indMac;
}
// For additions, if we already have a context, keep track of whether all offsets added
// to the address are constant, and their sum does not overflow.
Expand All @@ -8961,13 +8991,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA

tree->AsOp()->gtOp1 = op1 = fgMorphTree(op1, mac);

if ((mac != nullptr) && mac->m_used && tree->OperIsIndir())
{
// This context was used to elide an explicit null check on a FIELD_ADDR, with the expectation that
// this indirection will now be responsible for null-checking (implicitly).
tree->gtFlags &= ~GTF_IND_NONFAULTING;
}

// If we are exiting the "then" part of a Qmark-Colon we must
// save the state of the current copy assignment table
// so that we can merge this state with the "else" part exit
Expand Down

0 comments on commit ade2459

Please sign in to comment.