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

[JIT] Do not force creation of a new IG if the current IG has no instructions #89876

Merged
merged 17 commits into from
Aug 8, 2023

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Aug 2, 2023

#78891

Description

This PR simply makes it where we will not force a new IG if there are no instructions in the current one.

@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 Aug 2, 2023
@ghost ghost assigned TIHan Aug 2, 2023
@ghost
Copy link

ghost commented Aug 2, 2023

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

Issue Details

#78891

At the moment, this is just adding the test. I'm not able to reproducer it, so let's see what CI says.

Author: TIHan
Assignees: TIHan
Labels:

area-CodeGen-coreclr

Milestone: -

@TIHan TIHan marked this pull request as ready for review August 2, 2023 22:42
@TIHan
Copy link
Contributor Author

TIHan commented Aug 2, 2023

@dotnet/jit-contrib This is ready - @jakobbotsch - I added the test but it doesn't appear to fail CI - so it looks like the original issue is no longer an issue now.

@jakobbotsch
Copy link
Member

The problem still repros if you run with the following environment variables:

$env:DOTNET_JitDoVNBasedDeadStoreRemoval=0
$env:DOTNET_JitEnableEarlyLivenessRange=0
$env:DOTNET_EnableAVX2=0
$env:DOTNET_JitEnablePhysicalPromotion=0

@TIHan
Copy link
Contributor Author

TIHan commented Aug 3, 2023

Does it not reproduce with AltJit, only an actual linux machine?

DOTNET_AltJit=*M59*
DOTNET_AltJitName=clrjit_unix_x64_x64.dll
DOTNET_EnableAVX2=0
DOTNET_JitDoVNBasedDeadStoreRemoval=0
DOTNET_JitEnableEarlyLivenessRange=0
DOTNET_JitEnablePhysicalPromotion=0

Edit: I didn't set DOTNET_TieredCompilation=0, once I did that I got it to repro.

… enabling GC in the emitter, if the current IG has no instructions, do not force a new IG.
@TIHan TIHan changed the title Added issue 78891 regression test [JIT] Do not force creation of a new IG when enabling GC in the emitter if the current IG has no instructions Aug 7, 2023
@TIHan
Copy link
Contributor Author

TIHan commented Aug 7, 2023

@jit-contrib @BruceForstall This is ready again, pending CI.

@BruceForstall BruceForstall self-requested a review August 7, 2023 04:55
@BruceForstall
Copy link
Member

I will look at this, but I'm suspicious of the fix. The code currently doesn't force a new IG; it sets emitForceNewIG to defer creating a new IG until needed. If there's a case where the current IG is empty then I would expect the logic to not create a new IG should be put where the emitForceNewIG is handled.

@TIHan
Copy link
Contributor Author

TIHan commented Aug 7, 2023

If there's a case where the current IG is empty then I would expect the logic to not create a new IG should be put where the emitForceNewIG is handled.

I know where we can handle that check and actually did that before making this one.

@TIHan TIHan changed the title [JIT] Do not force creation of a new IG when enabling GC in the emitter if the current IG has no instructions [JIT] Do not force creation of a new IG if the current IG has no instructions Aug 7, 2023
@TIHan
Copy link
Contributor Author

TIHan commented Aug 7, 2023

@BruceForstall I changed it where we check if there are instructions before emitting the next IG.

@TIHan TIHan merged commit 3e293bc into dotnet:main Aug 8, 2023
@TIHan TIHan deleted the 78891-test branch August 8, 2023 02:43
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2023
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.

4 participants