Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[WIP] Alternative approach to issue 47202 #77370

Closed
wants to merge 16 commits into from

Conversation

BrianBohe
Copy link
Member

@BrianBohe BrianBohe commented Oct 24, 2022

This PR is intended to measure spmi results. It is another approach to solve issue #47202, in which we don't ask the vm for more memory than what is necessary, but we do scan all variable live ranges before copying them to the vm memory.

The thing is empty variable live ranges are useless. We are reporting them because they can get extended later if the variable becomes alive in the immediately next emitted instruction. If an empty live range is not getting extended, which we can realize after emitting all the code or creating a new live range for the same variable, we can remove it.

Tested running

<path_to_repo>\artifacts\tests\coreclr\windows.x64.Debug\Tests\Core_Root\corerun.exe  <path_to_repo>\artifacts\bin\coreclr\windows.x64.Debug\R2RDump\R2RDump.dll --in  <path_to_repo>\artifacts\bin\coreclr\windows.x64.Debug\System.Private.CoreLib.dll > output.txt

before and after this PR, checking the differences manually. Files are big and I wasn't able to get a nice diff that I could post here. I am open to any suggestions for more testing.

Brian Bohe added 8 commits October 20, 2022 14:23
The debugger is not using empty variable live ranges.
We are reporting them because they can get extended
later if the variable becomes alive in the immediately
next emitted instruction. If an empty live range is
not getting extended, which we can realize after
emitting all the code or creating a new live range
for the same variable, we can remove it.
When the emitter moved to the next group but has not
emitted any instruction, and the variable died and
becomes alive again, we would like to extend its range.
@ghost ghost assigned BrianBohe Oct 24, 2022
@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 Oct 24, 2022
@ghost
Copy link

ghost commented Oct 24, 2022

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

Issue Details

null

Author: BrianBohe
Assignees: BrianBohe
Labels:

area-CodeGen-coreclr

Milestone: -

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

I'm curious if all this logic can be done inside genSetScopeInfoUsingVariableRanges instead. There is already some existing logic there that extends previous live ranges, can we add some logic that handles this case there too and avoid the complicated emitLocation reasoning?

Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
@BrianBohe
Copy link
Member Author

I'm curious if all this logic can be done inside genSetScopeInfoUsingVariableRanges instead. There is already some existing logic there that extends previous live ranges, can we add some logic that handles this case there too and avoid the complicated emitLocation reasoning?

My guess is this is a little bit faster, but we can measure that with spmi in a new pr after this. Now we are deciding if we want to be faster and a little bit leaky in memory usage or be precise in memory usage but slower. What do you think?

There are cases in which we cannot tell whether two emitLocation are close enough to extend before emitting code, and even after emitting code we can potentially remove instructions, so we cannot avoid checking for extension in genSetScopeInfoUsingVariableRanges. Having said that, computing emitLocation's offset in genSetScopeInfoUsingVariableRanges is expensive, we may need to walk every instruction in its instruction group.

@jakobbotsch
Copy link
Member

My guess is this is a little bit faster, but we can measure that with spmi in a new pr after this. Now we are deciding if we want to be faster and a little bit leaky in memory usage or be precise in memory usage but slower. What do you think?

If we can centralize this kind of logic inside genSetScoepInfoUsingVariableRanges I would much prefer that. It is much easier to reason offsets about than comparing emitLocation in IGs given that IGs can be empty and that emitLocation can both point to one-after-the-end and the beginning of (any of the following) IGs.

There are cases in which we cannot tell whether two emitLocation are close enough to extend before emitting code, and even after emitting code we can potentially remove instructions, so we cannot avoid checking for extension in genSetScopeInfoUsingVariableRanges. Having said that, computing emitLocation's offset in genSetScopeInfoUsingVariableRanges is expensive, we may need to walk every instruction in its instruction group.

We are already reporting the offset in genSetScopeInfoUsingVariableRanges so the offset is already being computed and available there. Can we just add a simple case to the reportRange lambda there?

@BrianBohe BrianBohe changed the title [WIP] Issue 47202 [WIP] Alternative approach to issue 47202 Oct 25, 2022
@build-analysis build-analysis bot mentioned this pull request Oct 25, 2022
2 tasks
@BrianBohe
Copy link
Member Author

Closing this PR as its alternative performs better.

@BrianBohe BrianBohe closed this Oct 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 28, 2022
@BrianBohe BrianBohe deleted the issue-47202_approach2 branch February 10, 2023 23:06
# for free to subscribe to this conversation on GitHub. Already have an account? #.
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