Make compCurBB available for fgMorphBlockReturn.
#34184
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sandreenko
added
the
area-CodeGen-coreclr
added 5 commits
March 27, 2020 11:49
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?
sandreenko
force-pushed
the
extractMorphBlockReturn
branch
from
32adfb4
to
1f60a91
Compare
|
PTAL @dotnet/jit-contrib |
CarolEidt
approved these changes
Mar 28, 2020
| 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.
I realize this was pre-existing, but do you know what it means?
There was a problem hiding this comment.
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?
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were calling
tree = fgMorphCopyBlock(tree)infgMergeBlockReturnand it could calloptAssertionGen(asg)that would usecompCurBBin 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
compCurBBavailable forfgMorphBlockReturn.When we generate an assignment we could need to create a new assertion, that requires
compCurBBto be available.c9ee223: Delete
INVALID_POINTER_VALUE.I would like to remove it because:
compHndBBtabbecause 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.