Let GenTreeCopyOrReload handle scenarios when FEATURE_MULTIREG_RET is disabled #82451
Conversation
This file contains hidden or 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
ghost
assigned
Contributor
Author
|
@dotnet/jit-contrib @tannergooding |
tannergooding
approved these changes
Feb 22, 2023
Member
tannergooding
left a comment
tannergooding
left a comment
There was a problem hiding this comment.
LGTM as I understand the feature and associated code
SingleAccretion
approved these changes
Feb 22, 2023
Contributor
SingleAccretion
left a comment
SingleAccretion
left a comment
There was a problem hiding this comment.
LGTM as well with a small comment correction.
src/coreclr/jit/gentree.h
Outdated
|
|
||
| // Represents GT_COPY or GT_RELOAD node | ||
| // | ||
| // As it turns out, these are only needed on targets that happen to have multi-reg returns. |
Contributor
There was a problem hiding this comment.
Suggested change
| // As it turns out, these are only needed on targets that happen to have multi-reg returns. | |
| // Needed to support multi-reg ops. |
src/coreclr/jit/gentree.h
Outdated
| // As it turns out, these are only needed on targets that happen to have multi-reg returns. | ||
| // However, they are actually needed on any target that has any multi-reg ops. It is just | ||
| // coincidence that those are the same (and there isn't a FEATURE_MULTIREG_OPS). | ||
| // However, they are actually needed on any target that has any multi-reg ops. |
Contributor
There was a problem hiding this comment.
Suggested change
| // However, they are actually needed on any target that has any multi-reg ops. |
Otherwise the comments contradict themselves:
... are only needed on targets that happen to have multi-reg returns ...
... are actually needed on any target that has any multi-reg ops ...
Contributor
Author
|
/azp run runtime-coreclr superpmi-diffs |
Contributor
Author
|
/azp run runtime-coreclr superpmi-replay |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
Author
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Currently Windows ABI doesn't allow to split the result and return it in multiple registers. However, in #66551, we exposed an API that will let us return the result in multiple registers. The ABI doesn't allow us to do, but since
DivRemadded in #66551 is an intrinsic, we should continue to let the instruction return the result in multiple registers. Thanks @tannergooding for helping me understand this concept.GenTreeCopyOrReloadhad special code to handle scenario whenFEATURE_MULTIREG_RETwas allowed (on unix x64/arm64 and windows arm64) but this was breaking withDivRemsupport. The fix is to haveGenTreeCopyOrReloadalso handle scenarios whenFEATURE_MULTIREG_RETis disabled.Fixes: #82397