This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Implement GcInfo v2 for X86 #6974
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
swaroop-sridhar
added
* NO MERGE *
swaroop-sridhar
force-pushed
the
gc86
branch
2 times, most recently
from
edcade3 to
6bb8443
Compare
swaroop-sridhar
removed
the
* NO MERGE *
swaroop-sridhar
force-pushed
the
gc86
branch
4 times, most recently
from
baaf17b to
c2e452b
Compare
|
@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.
The following jobs are launched by default for each PR against dotnet/coreclr:master.
The following optional jobs are available in PRs against dotnet/coreclr:master.
Have a nice day! |
|
@dotnet-bot test OSX x64 Checked Build and Test |
|
@dotnet-bot test Windows_NT x86 ryujit Checked gcstress0x3 |
|
@dotnet-bot test Windows_NT x86 ryujit Checked gcstress0xc |
swaroop-sridhar
force-pushed
the
gc86
branch
from
c2e452b to
70f3a0a
Compare
|
@dotnet-bot test Windows_NT x86 ryujit Checked gcstress0x3 |
|
@dotnet-bot test Windows_NT x64 Checked gcstress0x3 |
1 similar comment
|
@dotnet-bot test Windows_NT x64 Checked gcstress0x3 |
|
@jkotas , @briansull: Please review |
|
What is the change in size in mscorlib, or other big managed assembly? Does this affect desktop? Have you done desktop testing? |
swaroop-sridhar
force-pushed
the
gc86
branch
from
70f3a0a to
6664b7a
Compare
|
A code size increase is expected from the change because of the extra two bytes to encode the correction to ReturnKind for all methods that have a GcInfo, and return a non-scalar. For Windows Release X86 RYU Jit: It is possible to regain some of this code size by:
|
|
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.
The following jobs are launched by default for each PR against dotnet/coreclr:master.
The following optional jobs are available in PRs against dotnet/coreclr:master.
Have a nice day! |
|
Yes, this change will affect Desktop too, because of the changes to the VM code. |
| size_t* prologSize) = 0; | ||
|
|
||
| /* | ||
| Returns true if the given IP is in the synchronized region of the method (valid for synchronized methods only) | ||
| */ | ||
| virtual bool IsInSynchronizedRegion( | ||
| DWORD relOffset, | ||
| PTR_VOID methodInfoPtr, | ||
| GCInfoToken gcInfoToken, |
swaroop-sridhar
force-pushed
the
gc86
branch
from
6664b7a to
64834af
Compare
|
@BruceForstall: I updated So, the desktop X86 JIT GCInfo is not affected by this change. I'll checkin the changes to Desktop Jit to its own branch separate from this change. I'm still running Desktop tests on this change, since there are some changes in common code. |
|
@dotnet-bot test Ubuntu gcstress0xc |
|
LGTM |
|
@janvorli Could you please take a look as well? |
|
@dotnet-bot test Windows_NT arm64 Checked |
swaroop-sridhar
force-pushed
the
gc86
branch
2 times, most recently
from
51fa362 to
16fcca9
Compare
| " %1d, %1d, %1d, %1d, %1d," | ||
| " %1d, %2d, %2d, %2d, %2d," | ||
| " %2d, %2d), \n", | ||
| " %1d, %1d, %1d, %1d, %1d, %1d," |
There was a problem hiding this comment.
The number of arguments in the format string and the number of actual value arguments doesn't match - there are 23 of them in the formatting string, but 28 actual arguments.
There was a problem hiding this comment.
Looks like there was a pre-existing mismatch. I'll fix it.
There was a problem hiding this comment.
This is fixed now, thanks.
swaroop-sridhar
force-pushed
the
gc86
branch
from
16fcca9 to
92c3d4f
Compare
| // | | | | | | | | | | | | | | | | | | | returnKind | ||
| // | | | | | | | | | | | | | | | | | | | | | ||
| // | | | | | | | | | | | | | | | | | | | | Arg count | ||
| // | | | | | | | | | | | | | | | | | | | | | Counted occurances |
There was a problem hiding this comment.
Since you are editing this file, could you please fix the spelling of the comment here: occurances -> occurences
This commit includes the following changes:
1) Thread GcInfo version through X86 specific APIs
2) Add ReturnKind and ReversePinvokeOffset fields to InfoHdr structure
GcInfo v1 and v2 use the same InfoHdr structures, because:
InfoHdrSmall: ReturnKind is encoded within previously unused bits.
InfoHdr: revPInvokeOffset will never be written to the image,
since ReversePinvokeOffset==INVALID_REV_PINVOKE_OFFSET for V1.
3) Update the Pre-computed header table to include bits for the above
[The default setting of ReturnKind=RT_Scalar is used for all entries in the table.
Optimizing this table based in most frequent usage scenarios is to be done separately]
4) Change the GC encoder/decoder to handle the above two fields
5) Use the ReturnKind in the GCInfo from thread-suspension code.
GcInfo version is changed for CoreCLR X86 only, not for Desktop JIT
Fixes #4379
swaroop-sridhar
force-pushed
the
gc86
branch
from
92c3d4f to
4871121
Compare
|
LGTM, thank you! |
briansull
reviewed
Sep 14, 2016
|
|
||
| BYTE nextByte = *table++; | ||
| BYTE encoding = nextByte & 0x7f; | ||
| const BYTE maskHaveMoreBytesBit = MORE_BYTES_TO_FOLLOW - 1; |
There was a problem hiding this comment.
This value is confusing: maskHaveMoreBytesBit
Instead of this why not define a second constant:
ENCODING_MASK = 0x7f
briansull
reviewed
Sep 14, 2016
| SET_UNTRACKED_MAX = 3, | ||
| SET_RET_KIND_MAX = 4, // 2 bits for ReturnKind | ||
| MORE_BYTES_TO_FOLLOW = 0x80 // If the High-bit of a header or adjustment byte | ||
| // is set, then there are more adjustments to follow. |
There was a problem hiding this comment.
@briansull your comments showed up in my browser after I merged.
I'll make the change in a separate checkin.
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.
Implement GcInfo v2 for X86
This commit includes the following changes:
GcInfo v1 and v2 use the same InfoHdr structures, because:
InfoHdrSmall: ReturnKind is encoded within previously unused bits.
InfoHdr: revPInvokeOffset will never be written to the image,
since ReversePinvokeOffset==INVALID_REV_PINVOKE_OFFSET for V1.
[The default setting of ReturnKind=RT_Scalar is used for all entries in the table.
Optimizing this table based in most frequent usage scenarios is to be done separately]
GcInfo version is changed for CoreCLR X86 only, not for Desktop JIT
Fixes #4379