This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Restore GT_INDEX_ADDR after #13682.#13701
Merged
Merged
Conversation
This restores the `GT_INDEX_ADDR` changes.
`genConsumeReg` marks the consumed register as not a GC pointer, as it assumes that the input register dies at the first instruction generated by the node. This is not the case for `INDEX_ADDR`, however, as the base register is multiply-used. As such, we need to mark the base regsiter as containing a GC pointer until we are finished generating the code for this node. Fixes
This function does not need to update the array info map when cloning a `GT_IND` if the address is a `GT_INDEX_ADDR`.
We were attempting to generate `base + index * size` using `MADD`, but had the registers in the wrong order and were generating `base * index + size`. This change fixes the register order s.t. the expected instruction is generated. Fixes #13593.
Author
|
@briansull @BruceForstall @CarolEidt @jkotas @dotnet/jit-contrib PTAL |
Author
|
@dotnet-bot help |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Author
|
@dotnet-bot |
This was referenced Aug 30, 2017
Author
|
@dotnet-bot test Windows_NT x64 corefx_minopts |
briansull
reviewed
Aug 30, 2017
| GenTree::s_gtNodeSizes[GT_FTN_ADDR] = TREE_NODE_SZ_LARGE; | ||
| GenTree::s_gtNodeSizes[GT_BOX] = TREE_NODE_SZ_LARGE; | ||
| GenTree::s_gtNodeSizes[GT_INDEX] = TREE_NODE_SZ_LARGE; | ||
| GenTree::s_gtNodeSizes[GT_INDEX_ADDR] = TREE_NODE_SZ_LARGE; |
Author
|
@dotnet-bot test OSX10.12 gcstress0x3 |
Author
|
CoreFX minopts failure is a build issue. |
briansull
reviewed
Aug 30, 2017
| return compRoot->m_arrayInfoMap; | ||
| } | ||
|
|
||
| inline bool TryGetArrayInfo(GenTreeIndir* indir, ArrayInfo* arrayInfo) |
There was a problem hiding this comment.
This method could use a function header explaining what it handles. (why it returns true vs. false etc...)
| if (tree->gtFlags & GTF_IND_ARR_INDEX) | ||
| { | ||
| ArrayInfo arrInfo; | ||
| if (TryGetArrayInfo(tree->AsIndir(), &arrInfo) && !tree->AsIndir()->gtOp1->OperIs(GT_INDEX_ADDR)) |
There was a problem hiding this comment.
Did you consider passing a argument stating whether we should cons up an ArrayInfo for the GT_INDEX_ADDR case.
Here we cons it up and discard it.
| if (indir->gtOp1->OperIs(GT_INDEX_ADDR)) | ||
| { | ||
| GenTreeIndexAddr* const indexAddr = indir->gtOp1->AsIndexAddr(); | ||
| *arrayInfo = ArrayInfo(indexAddr->gtElemType, indexAddr->gtElemSize, indexAddr->gtElemOffset, |
There was a problem hiding this comment.
Can you add a comment saying why we cons up an ArrayInfo here.
CarolEidt
approved these changes
Aug 30, 2017
CarolEidt
left a comment
CarolEidt
left a comment
There was a problem hiding this comment.
LGTM (aside from a couple of typos)
| genConsumeReg(index); | ||
|
|
||
| // NOTE: `genConsumeReg` marks the consumed register as not a GC pointer, as it assumes that the input registers | ||
| // die at the first instruction generated by the node. This is not the case for `INDEX_ADDR`, however, as the |
| genConsumeReg(index); | ||
|
|
||
| // NOTE: `genConsumeReg` marks the consumed register as not a GC pointer, as it assumes that the input registers | ||
| // die at the first instruction generated by the node. This is not the case for `INDEX_ADDR`, however, as the |
Author
|
@dotnet-bot test Tizen armel Cross Debug Build |
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 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.
These changes undo the reversion of the
GT_INDEX_ADDRchanges and fix a few bugs that were identified after the fact. Each bug fix is contained in a separate commit.Supersedes #13639 and #13665.
Fixes https://github.com/dotnet/corefx/issues/23586.