Fix dummy OBJ/BLK/IND nodes. #39824
Fix dummy OBJ/BLK/IND nodes. #39824
Conversation
sandreenko
added
the
area-CodeGen-coreclr
|
PTAL @erozenfeld @dotnet/jit-contrib |
src/coreclr/src/jit/liveness.cpp
Outdated
There was a problem hiding this comment.
That condition was pre-existing but it is not always correct, lower replaces many HWIntrinsic and other nodes as IND (for example Vector64.Create) without setting GTF_IND_NONFAULTING on these IND nodes. If you call fgDebugCheckFlags on such IND that is not marked as X and not marked GTF_IND_NONFAULTING it would fail.
A better condition would be if (!node->gtSetFlags() && (!node->OperMayThrow(this)) || ((node->gtFlags & GTF_EXCEPT) == 0) but such fix doesn't produce many positive diffs, so I would wait for
#10249 to show all such places and then fix the root cause, not the side-effects.
There was a problem hiding this comment.
I believe in LIR GTF_EXCEPT is not supposed to capture children side effects. What happens if you use if (!node->gtSetFlags() && ((node->gtFlags & GTF_EXCEPT) == 0) here?
There was a problem hiding this comment.
I believe in LIR GTF_EXCEPT is not supposed to capture children side effects
Yes, but we don't check it and right now many parent nodes are having GTF_EXCEPT from children, so with if (!node->gtSetFlags() && ((node->gtFlags & GTF_EXCEPT) == 0) we are keeping many null checks that we can delete.
There was a problem hiding this comment.
When you say "it is not always correct" you mean that it is overly conservative, correct?
It seems to me that the better approach would be to fix the setting of GTF_IND_NONFAULTING.
There was a problem hiding this comment.
When you say "it is not always correct" you mean that it is overly conservative, correct?
Yes.
It seems to me that the better approach would be to fix the setting of GTF_IND_NONFAULTING.
Agree, I should have said "a better temporary hack", I think we should set GTF_IND_NONFAULTING for 6.0 after #10249
src/coreclr/src/jit/liveness.cpp
Outdated
There was a problem hiding this comment.
I believe in LIR GTF_EXCEPT is not supposed to capture children side effects. What happens if you use if (!node->gtSetFlags() && ((node->gtFlags & GTF_EXCEPT) == 0) here?
src/coreclr/src/jit/liveness.cpp
Outdated
There was a problem hiding this comment.
Is it ok to have a dummy use of GT_IND?
There was a problem hiding this comment.
Yes, lower and rationalize tolerate that even for struct IND:
void Lowering::LowerIndir(GenTreeIndir* ind)
{
assert(ind->OperIs(GT_IND, GT_NULLCHECK));
// Process struct typed indirs separately unless they are unused;
// they only appear as the source of a block copy operation or a return node.
if (!ind->TypeIs(TYP_STRUCT) || ind->IsUnusedValue())
|
Any diffs from this change? |
No, there were a few functions with JitDump changes, but there were about |
src/coreclr/src/jit/liveness.cpp
Outdated
There was a problem hiding this comment.
We should probably factor this to a helper, e.g., ChangeOperToNullCheck but it can be done in a separate PR.
There was a problem hiding this comment.
I don't think this matters, but Lowering may change these to GT_IND because that produces better code for some cases.
There was a problem hiding this comment.
Yes, I saw that code, unconsumed IND are fine, we had asserts only about OBJ/BKJ/DYN_BLK.
There was a problem hiding this comment.
Right - my point was that it might be nice to avoid having to transform multiple times, but neither do we want to duplicate the determination of which form to use.
src/coreclr/src/jit/liveness.cpp
Outdated
There was a problem hiding this comment.
I don't think this matters, but Lowering may change these to GT_IND because that produces better code for some cases.
src/coreclr/src/jit/liveness.cpp
Outdated
There was a problem hiding this comment.
When you say "it is not always correct" you mean that it is overly conservative, correct?
It seems to me that the better approach would be to fix the setting of GTF_IND_NONFAULTING.
erozenfeld
left a comment
erozenfeld
left a comment
There was a problem hiding this comment.
LGTM with a couple of comment nits. Thank you for the refactoring.
* Add a repro. * Add a helper function `fgTryRemoveNonLocal`. * Fix the issue. * extract `gtChangeOperToNullCheck`. * update comments.
ghost
locked as resolved and limited conversation to collaborators
4 participants
If an unconsumed
OBJorBLKnode reachesrationalizeor later phases it fires an assert because we don't know how to produce them.Such nodes could appear from their parent removal if the parent is a dead store or an unused HWIntrinsic intrinsic.
This PR fixes #39737, there are other issues that were discovered but they are CQ and don't meet RC1 bar.
Changes:
a1697de: Add a repro.
0158523: Add a helper function
fgTryRemoveNonLocal.4ee3da9: Fix the issue.