-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Forbid creation of non-faulting null-check nodes. #77078
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsDraft to test #44227.
|
Fixed all asserts, but there are size and throughput regressions on Arm64. Link @AndyAyersMS @kunalspathak PTAL. |
src/coreclr/jit/lower.cpp
Outdated
const bool isContainable = false; | ||
if (ind->Addr() != nullptr) | ||
{ | ||
const bool isContainable = IsSafeToContainMem(ind, ind->Addr()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is hiding the other isContainable
and then going out of scope and the value is lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep and this is the reason for so many diffs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I forgot to remove it when moving it out of if. Thanks for spotting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a nit here - you could write it as
const bool isContainable = (ind->Addr() != nullptr) && IsSafeToContainMem(ind, ind->Addr());
if you wanted to preserve the const
ness or shorten it. I don't think it's a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the code snippet. Will make that change.
@JulieLeeMSFT This was stale; I converted to Draft. Please convert back to non-Draft when it's ready for final code reviews. |
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
|
@dotnet/jit-contrib @BruceForstall, it is ready to review. |
src/coreclr/jit/importer.cpp
Outdated
@@ -8251,7 +8251,14 @@ void Compiler::impImportBlockCode(BasicBlock* block) | |||
// via an underlying address, just null check the address. | |||
if (op1->OperIs(GT_FIELD, GT_IND, GT_OBJ)) | |||
{ | |||
gtChangeOperToNullCheck(op1, block); | |||
if (fgAddrCouldBeNull(op1->AsUnOp()->gtGetOp1())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The op1
(addr) of a static GT_FIELD can be nullptr
, but fgAddrCouldBeNull
doesn't check for that. Could we AV here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said, if the op1 address of GT_FIELD can be nullptr
, we want true
returned from fgAddrCouldBeNull(op1->gtGetOp1())
. And, it falls through to the default case and returns true
. Isn't it enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because fgAddrColdBeNull()
has switch (addr->OperGet())
and in the case I'm worried about, addr
will be nullptr, so that would AV. Now, maybe there's some reason the case I'm worried about doesn't happen.
Or, maybe you can rewrite it to protect against that, e.g.:
const GenTree* addr = op1->gtGetOp1();
if ((addr != nullptr) && fgAddrCouldBeNull(addr))
{
gtChangeOperToNullCheck(op1, block);
}
else
{
op1 = gtNewNothingNode();
}
(presumably static fields don't need null checks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This pull request has been automatically marked |
@BruceForstall, it is ready to review. |
src/coreclr/jit/gentree.cpp
Outdated
else | ||
{ | ||
JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NOTHING node\n", dspTreeID(node)); | ||
node = m_compiler->gtNewNothingNode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that Append(node)
here happens before you set node = m_compiler->gtNewNothingNode();
. The NothingNode gets created then never used. And for the non-faulting case, the original BLK node is still on the side-effect list.
I think you want code more like this:
if (m_compiler->gtNodeHasSideEffects(node, m_flags))
{
if (node->OperIsBlk() && !node->OperIsStoreBlk())
{
// Check for a guaranteed non-faulting IND, and create a NOP node instead of a NULLCHECK in that case.
if (m_compiler->fgAddrCouldBeNull(node->AsBlk()->Addr()))
{
Append(node);
JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node));
m_compiler->gtChangeOperToNullCheck(node, m_compiler->compCurBB);
}
else
{
JITDUMP("Dropping non-faulting OBJ/BLK node [%06d]\n", dspTreeID(node));
}
}
else
{
Append(node);
}
return Compiler::WALK_SKIP_SUBTREES;
}
src/coreclr/jit/importer.cpp
Outdated
@@ -8251,7 +8251,14 @@ void Compiler::impImportBlockCode(BasicBlock* block) | |||
// via an underlying address, just null check the address. | |||
if (op1->OperIs(GT_FIELD, GT_IND, GT_OBJ)) | |||
{ | |||
gtChangeOperToNullCheck(op1, block); | |||
if (fgAddrCouldBeNull(op1->AsUnOp()->gtGetOp1())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because fgAddrColdBeNull()
has switch (addr->OperGet())
and in the case I'm worried about, addr
will be nullptr, so that would AV. Now, maybe there's some reason the case I'm worried about doesn't happen.
Or, maybe you can rewrite it to protect against that, e.g.:
const GenTree* addr = op1->gtGetOp1();
if ((addr != nullptr) && fgAddrCouldBeNull(addr))
{
gtChangeOperToNullCheck(op1, block);
}
else
{
op1 = gtNewNothingNode();
}
(presumably static fields don't need null checks)
…des that was supposed to be deleted.
@BruceForstall, it is ready to review. |
Fixes #44227.
There are 2 methods that create
GT_NULLCHECK
:gtNewNullCheck
andgtChangeOperToNullCheck
.assert(fgAddrCouldBeNull(addr))
togtNewNullCheck
to avoid unnecessary null-checks early.assert(fgAddrCouldBeNull(tree->AsUnOp()->gtGetOp1()))
togtChangeOperToNullCheck
using @sandreenko's prototype.Edit: To delete an unnecessary node, we replaced the node with a
GT_NULLCHECK
orGT_NOP
and allowed later phases to delete it.