Skip to content

Fix for number of used frame infos during crossgen2 compilation #64549

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

Closed
gbalykov wants to merge 1 commit into from
Closed

Fix for number of used frame infos during crossgen2 compilation #64549

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3430,6 +3430,7 @@ private void reportFatalError(CorJitResult result)
{
// We could add some logging here, but for now it's unnecessary.
// CompileMethod is going to fail with this CorJitResult anyway.
Copy link
Member

@jkotas jkotas Mar 14, 2022

Choose a reason for hiding this comment

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

This comment is not correct. CompileMethod is not always going to fail.

Copy link
Member

@jkotas jkotas Mar 14, 2022

Choose a reason for hiding this comment

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

This comment is not correct. CompileMethod is not always going to fail.

_numFrameInfos = 0;
Copy link
Member

@jkotas jkotas Mar 14, 2022

Choose a reason for hiding this comment

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

Is this all state that needs to be cleaned up? From cursory look, there seems to be more state that may cause problems.

I am wondering whether the retry logic should be rather moved from the JIT to the EE side. It would allow the state cleanup to be more robust. Also, it would allow us to skip the retry in cases where it does not make sense. For example, it does not make sense to retry with no optimizations when generating Tier1 code.

@dotnet/jit-contrib Thoughts?

Copy link
Contributor

@BruceForstall BruceForstall Mar 15, 2022

Choose a reason for hiding this comment

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

I am wondering whether the retry logic should be rather moved from the JIT to the EE side.

Maybe I'm missing something: the proposed change is in crossgen2, not the JIT (or EE).

In the JIT'ing case, we already have a backout function: CEEJitInfo::BackoutJitData() => EEJitManager::RemoveJitData(). It removes EH info, GC info, and maybe more. (Maybe not the debug info?) Seems like crossgen2 would need to do the same (i.e., remove GC info, debug info, code buffer).

Copy link
Member

@jkotas jkotas Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We have similar method in crossgen2 as well (CompileMethodCleanup). The problem is that neither BackoutJitData nor CompileMethodCleanup get called when the JIT decides to retry with optimizations off.

Copy link
Contributor

@BruceForstall BruceForstall Mar 15, 2022

Choose a reason for hiding this comment

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

Ah, I see. There's a "small" window where, after the JIT has already started allocating EH/GC/code space from the VM, where it can hit a NO_WAY assert and decide to retry. It would make sense for the JIT to call back through the JIT/EE interface before retrying saying "I'm going to retry".

Copy link
Member

@jakobbotsch jakobbotsch Mar 15, 2022

Choose a reason for hiding this comment

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

Personally I think it makes sense to leave it up to the consumer of the JIT to retry if that is the desired behavior -- it seems a little strange to me that the JIT does this implicitly even though optimizations were requested. It was also the reason behind the unexpected debug codegen in #63708.

Copy link
Member

@jkotas jkotas Mar 15, 2022

Choose a reason for hiding this comment

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

reportFatalError signals that the JIT is going to retry, so we sort of have that callback already.

There is retry logic in the VM to deal with relocs overflow that does proper cleanup before retrying. In other words, we have one retry logic in the JIT itself and second retry logic in the VM. My point was whether it would be better to have just one retry logic in the VM that handles both cases.

Copy link
Member

@jkotas jkotas Mar 14, 2022

Choose a reason for hiding this comment

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

Is this all state that needs to be cleaned up? From cursory look, there seems to be more state that may cause problems.

I am wondering whether the retry logic should be rather moved from the JIT to the EE side. It would allow the state cleanup to be more robust. Also, it would allow us to skip the retry in cases where it does not make sense. For example, it does not make sense to retry with no optimizations when generating Tier1 code.

@dotnet/jit-contrib Thoughts?

}

private void recordCallSite(uint instrOffset, CORINFO_SIG_INFO* callSig, CORINFO_METHOD_STRUCT_* methodHandle)
Expand Down