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

Switch IP mapping lists to use jitstd::list #61822

Merged
merged 2 commits into from
Nov 19, 2021

Conversation

jakobbotsch
Copy link
Member

We may want to change the logic around how IP mappings are reported to
the EE, and the manually maintained singly-linked lists made it hard to
understand the logic that goes on here. Switch to jitstd::list which
allows us to simplify the logic by directly removing the mappings we do
not want to emit.

There are two small behavior changes here:

  1. The previous logic would record superfluous IL offset 0 mappings
    under the assumption that the previous mapping was the prolog; this
    is not always the case, so check this explicitly
  2. The previous logic would record all CALL_INSTRUCTION mappings and
    would not use these to check whether to remove NO_MAP mappings. This
    is superfluous as well, if we have a regular mapping at a native
    offset it does not give any information to add a NO_MAP mapping at
    the same native offset.

All diffs I looked at therefore look like the following. In case 1 above:

-IP mapping count : 7
+IP mapping count : 6
 IL offs PROLOG : 0x00000000 ( STACK_EMPTY )
 IL offs 0x0000 : 0x00000009 ( STACK_EMPTY )
 IL offs 0x0000 : 0x000000F4 ( STACK_EMPTY )
-IL offs 0x0000 : 0x000000F4 ( STACK_EMPTY )
 IL offs NO_MAP : 0x00000114 ( STACK_EMPTY )
 IL offs EPILOG : 0x0000011D ( STACK_EMPTY )
 IL offs 0x0000 : 0x00000124 ( STACK_EMPTY )

And in case 2:

 IL offs 0x0000 : 0x000000D7 ( STACK_EMPTY )
 IL offs 0x0001 : 0x000000DE ( CALL_INSTRUCTION )
-IL offs NO_MAP : 0x000000E9 ( STACK_EMPTY )
 IL offs 0x000B : 0x000000E9 ( CALL_INSTRUCTION )
 IL offs NO_MAP : 0x000000F7 ( STACK_EMPTY )

We may want to change the logic around how IP mappings are reported to
the EE, and the manually maintained singly-linked lists made it hard to
understand the logic that goes on here. Switch to jitstd::list which
allows us to simplify the logic by directly removing the mappings we do
not want to emit.

There are two small behavior changes here:
1. The previous logic would record superfluous IL offset 0 mappings
   under the assumption that the previous mapping was the prolog; this
   is not always the case, so check this explicitly
2. The previous logic would record _all_ CALL_INSTRUCTION mappings and
   would not use these to check whether to remove NO_MAP mappings.  This
   is superfluous as well, if we have a regular mapping at a native
   offset it does not give any information to add a NO_MAP mapping at
   the same native offset.

All diffs therefore look like the following. Case 1:
-IP mapping count : 7
+IP mapping count : 6
 IL offs PROLOG : 0x00000000 ( STACK_EMPTY )
 IL offs 0x0000 : 0x00000009 ( STACK_EMPTY )
 IL offs 0x0000 : 0x000000F4 ( STACK_EMPTY )
-IL offs 0x0000 : 0x000000F4 ( STACK_EMPTY )
 IL offs NO_MAP : 0x00000114 ( STACK_EMPTY )
 IL offs EPILOG : 0x0000011D ( STACK_EMPTY )
 IL offs 0x0000 : 0x00000124 ( STACK_EMPTY )

Case 2:
 IL offs 0x0000 : 0x000000D7 ( STACK_EMPTY )
 IL offs 0x0001 : 0x000000DE ( CALL_INSTRUCTION )
-IL offs NO_MAP : 0x000000E9 ( STACK_EMPTY )
 IL offs 0x000B : 0x000000E9 ( CALL_INSTRUCTION )
 IL offs NO_MAP : 0x000000F7 ( STACK_EMPTY )
@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 Nov 19, 2021
@ghost
Copy link

ghost commented Nov 19, 2021

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

Issue Details

We may want to change the logic around how IP mappings are reported to
the EE, and the manually maintained singly-linked lists made it hard to
understand the logic that goes on here. Switch to jitstd::list which
allows us to simplify the logic by directly removing the mappings we do
not want to emit.

There are two small behavior changes here:

  1. The previous logic would record superfluous IL offset 0 mappings
    under the assumption that the previous mapping was the prolog; this
    is not always the case, so check this explicitly
  2. The previous logic would record all CALL_INSTRUCTION mappings and
    would not use these to check whether to remove NO_MAP mappings. This
    is superfluous as well, if we have a regular mapping at a native
    offset it does not give any information to add a NO_MAP mapping at
    the same native offset.

All diffs I looked at therefore look like the following. In case 1 above:

-IP mapping count : 7
+IP mapping count : 6
 IL offs PROLOG : 0x00000000 ( STACK_EMPTY )
 IL offs 0x0000 : 0x00000009 ( STACK_EMPTY )
 IL offs 0x0000 : 0x000000F4 ( STACK_EMPTY )
-IL offs 0x0000 : 0x000000F4 ( STACK_EMPTY )
 IL offs NO_MAP : 0x00000114 ( STACK_EMPTY )
 IL offs EPILOG : 0x0000011D ( STACK_EMPTY )
 IL offs 0x0000 : 0x00000124 ( STACK_EMPTY )

And in case 2:

 IL offs 0x0000 : 0x000000D7 ( STACK_EMPTY )
 IL offs 0x0001 : 0x000000DE ( CALL_INSTRUCTION )
-IL offs NO_MAP : 0x000000E9 ( STACK_EMPTY )
 IL offs 0x000B : 0x000000E9 ( CALL_INSTRUCTION )
 IL offs NO_MAP : 0x000000F7 ( STACK_EMPTY )
Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM with one nit

src/coreclr/jit/compiler.cpp Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants