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.

Sign up for GitHub

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

Jump to bottom

Support nocheck prefix #81615

Closed
wants to merge 19 commits into from
Closed

Support nocheck prefix #81615

wants to merge 19 commits into from

Conversation

john-h-k
Copy link
Contributor

@john-h-k john-h-k commented Feb 3, 2023 •
edited
Loading

Another shot at #35491

Supports the no. prefix as a nop

I have a separate branch implementing it but I think by splitting it up it will be a bit more simple.

Fixes #72695

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 3, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 3, 2023
@ghost
Copy link

ghost commented Feb 3, 2023

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

Issue Details

Another shot at #35491

Supports the no. prefix as a nop

I have a separate branch implementing it but I think by splitting it up it will be a bit more simple.

Author: john-h-k
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution
Milestone: -

@john-h-k
Copy link
Contributor Author

john-h-k commented Feb 3, 2023

This is currently blocked as someone with the Microsoft internal YACC tool needs to regenerate asmparse.cpp

@JulieLeeMSFT
Copy link
Member

Thanks @john-h-k, @jakobbotsch will help you on this.

This is currently blocked as someone with the Microsoft internal YACC tool needs to regenerate asmparse.cpp

@jakobbotsch
Copy link
Member

@john-h-k I've regenerated the parser with a small fix in the grammar. Can you test whether it works as expected? We should also verify that roundtripping with ildasm works correctly.

This was referenced Feb 17, 2023
Closed
Closed
@JulieLeeMSFT
Copy link
Member

@john-h-k, can you please address feedback from @jakobbotsch?

@JulieLeeMSFT JulieLeeMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 3, 2023
@JulieLeeMSFT
Copy link
Member

@john-h-k, checking back if you will work on this PR soon. If it is not handled for too long, this PR will be closed automatically.

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 25, 2023
@john-h-k
Copy link
Contributor Author

@JulieLeeMSFT Yes this is still WIP, have addressed @jakobbotsch 's comments

@jakobbotsch jakobbotsch reopened this May 15, 2023
jakobbotsch
jakobbotsch reviewed May 15, 2023
View reviewed changes
src/coreclr/jit/importer.cpp
Comment on lines +8802 to +8805
// PREFIX_NO_TYPECHECK = 0x000000040 which is 0x1 (the value of typecheck flag) << 6
// The 2 subsequent flags are just left shift of 1, the same as the nullcheck/rangecheck IL flags, so we just have to
// left shift by 6 to transform
flags <<= 6;
Copy link
Member

Choose a reason for hiding this comment

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

This should be changed in the same way as fgFindJumpTargets.

jakobbotsch
jakobbotsch reviewed May 15, 2023
View reviewed changes
src/coreclr/jit/importer.cpp
Comment on lines +8812 to +8814



Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

jakobbotsch
jakobbotsch reviewed May 15, 2023
View reviewed changes
src/coreclr/vm/readytorunstandalonemethodmetadata.cpp
Comment on lines -96 to -98
case CEE_UNUSED69: // This is the no. prefix that is partially defined in Partition III.
SkipIL(1);
break;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we still skip 1 byte due to the flags?

@jakobbotsch jakobbotsch added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 6, 2023
@BruceForstall
Copy link
Member

@john-h-k did you see the comments from @jakobbotsch ?

@ghost ghost added the no-recent-activity label Jun 26, 2023
@ghost
Copy link

ghost commented Jun 26, 2023

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@john-h-k
Copy link
Contributor Author

Have seen the comments and will address ASAP

@ghost ghost removed needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity labels Jun 26, 2023
@jakobbotsch jakobbotsch added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 24, 2023
@TIHan
Copy link
Contributor

TIHan commented Aug 3, 2023

@john-h-k following up - what is your status with this PR?

@ghost ghost added the no-recent-activity label Aug 17, 2023
@ghost
Copy link

ghost commented Aug 17, 2023

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@ghost
Copy link

ghost commented Sep 1, 2023

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

@ghost ghost closed this Sep 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jakobbotsch jakobbotsch jakobbotsch left review comments

Assignees

@jakobbotsch jakobbotsch

@john-h-k john-h-k

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ILDAsm and ILAsm don't handle the no. prefix
5 participants
@john-h-k @JulieLeeMSFT @jakobbotsch @BruceForstall @TIHan