Skip to content

[mini] Fix typo in mono_decompose_vtype_opts #90825

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

Merged
merged 2 commits into from
Aug 19, 2023

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Aug 18, 2023

Without this, if some previous instruction already created a vreg for ins->dest (for example if we are doing multiple passes over the basic block because restart == TRUE) we will use an incorrect vreg for the src when decomposing the current VMOVE


Additionally, only emit OP_LDTOKEN_FIELD if we loaded a field token

This is used by a CreateSpan intrinsic #81695 that needs access to the MonoClassField*

For other cases of a bare LDTOKEN (such as hand-written IL that calls LDTOKEN on a type but doesn't follow it up with a call to GetTypeFromHandle leave the opcode as a VMOVE (from the EMIT_NEW_TEMPLOAD above))

Fixes #90800

Without this, if some previous instruction already created a vreg for
ins->dest (for example if we are doing multiple passes over the basic
block because `restart == TRUE`) we will use an incorrect vreg when
decomposing the current VMOVE

Fixes dotnet#90800
This is used by a CreateSpan optimization that needs access to the
MonoClassField*

For other cases of a bare LDTOKEN (such as hand-written IL that calls
LDTOKEN on a type but doesn't follow it up with a call to
`GetTypeFromHandle` leave the opcode as a VMOVE (from the
`EMIT_NEW_TEMPLOAD` above))
@lambdageek
Copy link
Member Author

TODO: need to add a regression test.

attn @SamMonoRT we will need to backport this to net8.

@lambdageek
Copy link
Member Author

@simonrozsival @rolfbjarne do we need to try to get this into rc1, is it completely blocking ios?

As a workaround, emitting roslyn-style IL will avoid this issue (because there will be an intervening callvirt that will avoid the decompose bug)
https://sharplab.io/#v2:C4LglgNgPgAgTARgLACgDEA7ArhCBDAIwgFMACAEzAGdCTVSHTUYBmU+UgYVIG97HWpAEpYMwMAFtiAFQCeAB2IAJPBnIlSAWQAUASl79GAgOylgC4gHsAZtpgIADLoB0cxSrUkA3IdIBfVD8gA=

ie instead of ldtoken System.Runtime]System.String emit

        IL_0000: ldtoken [System.Runtime]System.String
        IL_0005: call class [System.Runtime]System.Type [System.Runtime]System.Type::GetTypeFromHandle(valuetype [System.Runtime]System.RuntimeTypeHandle)
        IL_000a: callvirt instance valuetype [System.Runtime]System.RuntimeTypeHandle [System.Runtime]System.Type::get_TypeHandle()

@lambdageek lambdageek requested a review from jandupej August 18, 2023 20:58
@lambdageek lambdageek added this to the 8.0.0 milestone Aug 18, 2023
@lewing
Copy link
Member

lewing commented Aug 18, 2023

if this is a blocker for rc1 we should open the backport now and send the email

@lambdageek
Copy link
Member Author

/backport to release/8.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5907349234

@lambdageek
Copy link
Member Author

lambdageek commented Aug 18, 2023

@lewing I'm offline for at least another 3 hours. Can you send the email - see the comments in the issue for details I sent the email. Waiting on a decision...

@lewing lewing merged commit 940b332 into dotnet:main Aug 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Mono] RuntimeTypeHandle.Equals seems to be broken in Release mode
3 participants