Skip to content
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

[interp] handle box + brtrue/brfalse and box+isinst+unbox.any patterns up front #103052

Merged
merged 7 commits into from
Jun 9, 2024

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Jun 4, 2024

In cases where we don't do cprop and deadce (for example if mono_interp_opt is 0 because we're debugging) don't emit a box_vt opcode before a branch. Instead just emit a constant true

Fixes #102988

Also reverts the changes from #102998

In cases where we don't do cprop and deadce (for example if
~mono_interp_opt~ is 0 because we're debugging) don't emit a box_vt
opcode before a branch.  Instead just emit a constant true

Fixes dotnet#102988
Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

@BrzVlad
Copy link
Member

BrzVlad commented Jun 5, 2024

This means that the previous approach of retrying with optimization enabled is useless and should be removed. We should handle all IL patterns like this.

Interp changes LGTM.

@lambdageek
Copy link
Member Author

@BrzVlad PTAL. I updated the interpreter to handle the other patterns in the unoptimized pass.

@AaronRobinsonMSFT I added a new test locking down the behavior for box S1 ; isinst S2 ; unbox.any S2 - CoreCLR and Mono mini both throw InvalidProgramException which seems reasonable - as we discussed Roslyn seems to only ever emit this sequence in box S1 ; isinst S2; brtrue/brfalse branches where the types match.

@stephentoub this PR also reverts your previous revert

@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 9.0.0 milestone Jun 7, 2024
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
interp_add_ins (td, MINT_LDC_I4_S);
td->last_ins->data[0] = (guint16) 1;
push_simple_type (td, STACK_TYPE_I4);
interp_ins_set_dreg (td->last_ins, td->sp [-1].var);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the [-1] a comment mechanic in the intepreter? is there a macro or some other abstraction over that? I'm assuming the sp is "stack pointer", so perhaps that is the previous slot?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I'm not sure why we do it this way. But the general pattern is to decrement SP by the number of operands that an opcode expects (so now they're at sp[0] ... sp[k-1]) and use those vars, then do a push for the result(s) (which creates new vars) and then use sp[-1] to refer to the result.

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.

[WASM] browser-wasm tests fail with "Cannot box IsByRefLike type"
3 participants