-
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
Make compCurBB
available for fgMorphBlockReturn
.
#34184
Conversation
Morph is already a very vague verb in the Jit, try to use a more precise one.
When we generate an asignment we could need to create a new assertion, that requires `compCurBB` to be available.
I would like to remove it because: 1) it was debug only; 2) there were no null checks for `compHndBBtab`, because it is a dependent variable so there was no need to ditinguish valid null pointer from a bad invalid pointer; 3) that is the only place where this mechanism was used.
I can't see a reason why it should not, there are no diffs. The issue with the previous version was that we did not actually know what we were marking: GT_ASG, GT_COMMA, something else? Was the idea to mark individual ASG under COMMA?
32adfb4
to
1f60a91
Compare
PTAL @dotnet/jit-contrib |
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.
LGTM
assert((block->bbJumpKind == BBJ_RETURN) && ((block->bbFlags & BBF_HAS_JMP) == 0)); | ||
assert((genReturnBB != nullptr) && (genReturnBB != block)); | ||
|
||
// TODO: Need to characterize the last top level stmt of a block ending with BBJ_RETURN. |
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.
I realize this was pre-existing, but do you know what it means?
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.
I do not know, I guess the idea was to describe different patterns that we can see as the last STMT in a BBJ_RETURN
, they are probably GT_RETURN
tree, GT_CALL (for a tail call)
, and anything else for a void return method?
We were calling
tree = fgMorphCopyBlock(tree)
infgMergeBlockReturn
and it could calloptAssertionGen(asg)
that would usecompCurBB
in several places likeruntime/src/coreclr/src/jit/assertionprop.cpp
Line 2731 in d6e3a0f
We have not seen a failure because we did not have struct returns and
tree->OperIsCopyBlkOp()
check was always false.No diffs x64(crossgen, pmi, SPMI).
Easier to review by commits with whitespace changes disables:
04eccab: Extract
fgMergeBlockReturn
.I wanted to call it
fgMorphBlockReturn
, but morph is already a very vague verb in the Jit, tried to use a more precise one.315288b: Add a function header.
8347f1c: Make
compCurBB
available forfgMorphBlockReturn
.When we generate an assignment we could need to create a new assertion, that requires
compCurBB
to be available.c9ee223: Delete
INVALID_POINTER_VALUE
.I would like to remove it because:
compHndBBtab
because it is a dependent variableso there was no need to distinguish valid null pointer from a bad invalid pointer;
1f60a91: Allow to CSE the merge return ASG.
I can't see a reason why it should not be allowed.
The issue with the previous version was that we did not actually know what we were marking: GT_ASG, GT_COMMA, something else? Was the idea to mark individual ASG under COMMA?
Failures are unrelated.
Preparation for #33225.