JIT: Avoid reporting call-sites in MinOpts on all targets #103950

jakobbotsch · 2024-06-25T09:07:44Z

When the call-sites have no interesting GC information we avoid reporting them, but this behavior was enabled only for x86/x64. Enable this on other platforms too.

Fix #103917

When the call-sites have no interesting GC information we avoid reporting them, but this behavior was enabled only for x86/x64. Enable this on other platforms too. Fix dotnet#103917

jakobbotsch · 2024-06-25T09:09:04Z

src/coreclr/jit/gcencode.cpp

@@ -4065,13 +4065,7 @@ void GCInfo::gcMakeRegPtrTable(
 {
    GCENCODER_WITH_LOGGING(gcInfoEncoderWithLog, gcInfoEncoder);

-    // TODO: Decide on whether we should enable this optimization for all
-    // targets: https://github.com/dotnet/runtime/issues/103917
-#ifdef TARGET_XARCH
    const bool noTrackedGCSlots = compiler->opts.MinOpts() && !compiler->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT);


@jkotas do you know if there is any dependency on the call site locations by crossgen2? I'm not sure if the PREJIT condition can also be removed here.

I do not think crossgen2 has any dependency on that. NativeAOT might have (unintentional) dependency on it - you may want to run NativeAOT outer loop.

The primary motivation for this feature was JIT throughput improvement. This condition is likely result of that thinking - spending the extra cycles for AOT is no big deal.

I do not see a problem with removing this condition.

The change in https://github.com/dotnet/runtime/pull/102680/files#r1653150895 may make us sensitive to missing safepoints.

Mostly just a matter of changing asserts though. We know the caller site is GC-safe. (since we are in a GC-capable callee)

Do you want me to revert the PREJIT change for now?

…call-sites

jakobbotsch · 2024-06-25T15:06:45Z

/azp run runtime-nativeaot-outerloop, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-06-25T15:07:05Z

Azure Pipelines successfully started running 2 pipeline(s).

jakobbotsch · 2024-07-09T11:40:18Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-07-09T11:40:34Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2024-07-10T11:22:49Z

/azp run runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2024-07-10T11:23:08Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2024-10-31T15:08:11Z

/azp run runtime-nativeaot-outerloop, runtime-coreclr gcstress0x3-gcstress0xc

VSadov · 2024-10-31T20:37:03Z

I think NativeAOT at this point should be compatible with this change. This mostly affects decoder and that is shared.
I was waiting for gc stress tests to maybe find something.

It may be worth pushing a temporary change that unconditionally enables minopts for naot to see whether there are any real issues.

I think it would be interesting. Maybe we will find issues related to this change or even unrelated ones…

jakobbotsch · 2024-11-04T11:46:52Z

@jakobbotsch It may be worth pushing a temporary change that unconditionally enables minopts for naot to see whether there are any real issues.

Do you know what the simplest way to do that would be? I imagine I can't just do it on the JIT side due to IL scanner mismatches?

jkotas · 2024-11-04T17:56:49Z

You should be able to do it in the JIT. IL scanner is not aware of minopts vs. full opts differences. It has to assume the worst case. It has no control over when the JIT decides to degrade to minopts on its own.

jakobbotsch · 2024-11-05T10:13:17Z

Hmm, that didn't seem to work. Any other ideas?

jkotas · 2024-11-05T17:04:59Z

This shows that there are pre-existing issues with minopts and IL scanner. cc @MichalStrehovsky

MichalStrehovsky · 2024-11-06T06:02:29Z

This shows that there are pre-existing issues with minopts and IL scanner. cc @MichalStrehovsky

The scanner makes an assumption that NI_System_Runtime_InteropService_MemoryMarshal_GetArrayDataReference gets expanded. Submitted #109576 to make this easier to diagnose.

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs

Lines 434 to 437 in 3116db9

    
           if (IsMemoryMarshalGetArrayDataReference(method)) 
        
           { 
        
               return; 
        
           }

If I mark this as betterToExpand in RyuJIT, smoke tests can build fine with this change. Any opinions on doing this unconditionally, or on native AOT only?

Smoke tests don't run with this change however, we're hitting assert(decoder.IsSafePoint()); in CoffNativeCodeManager.cpp so there's probably some problems.

VSadov · 2024-11-06T18:01:59Z

Smoke tests don't run with this change however, we're hitting assert(decoder.IsSafePoint()); in CoffNativeCodeManager.cpp so there's probably some problems.

This is a bit strange. This is an assert in a leaf frame checking that we were interrupted on a safe point.

This change indeed removes safe points from the GC info, but then how could the execution be interrupted if the safe point is missing?

VSadov · 2024-11-06T18:05:41Z

but then how could the execution be interrupted if the safe point is missing?

Ah, yes, it could be interrupted via hijacking.
The assert should be changed to assert(decoder.CouldBeInterruptibleSafePoint()

The helper was specifically introduced to be more permissive in asserts and allow cases where safepoints are possibly missing due to MinOpts.

jkotas · 2024-11-06T22:31:26Z

If I mark this as betterToExpand in RyuJIT, smoke tests can build fine with this change. Any opinions on doing this unconditionally, or on native AOT only?

I think we should do it unconditionally. (It would be more correct to treat it as mustToExpand, but we seem to depend on betterToExpand to mean mustToExpand with IL scanner for other methods, so it is not introducing a new fundamental problem.)

@jakobbotsch Are you fine with doing this and the change that @VSadov suggested in this PR, or would you like those changes to be submitted separately?

jakobbotsch · 2024-11-07T10:33:50Z

@jakobbotsch Are you fine with doing this and the change that @VSadov suggested in this PR, or would you like those changes to be submitted separately?

I think we should fix the ILScanner/GetArrayDataReference issue separately since it's not really related to this change, but @VSadov's issue seems related so makes sense to fix it here.

If you think we should expand this unconditionally the intrinsic should just be made recursive in the C# sources, right? (Hmm, no.. we don't really have prior art to unconditionally expanded intrinsics in callers outside of NAOT. I would probably suggest we just add this one to the list of ones expanded for NAOT.)

…call-sites

jakobbotsch · 2024-11-14T11:09:37Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-11-14T11:09:55Z

Azure Pipelines successfully started running 1 pipeline(s).

VSadov · 2024-11-28T00:37:06Z

There are bunch of failures on NativeAOT that complain about size increase:

BUG: File size is not in the expected range (1228800 to 1638400 bytes). Did a libraries change regress size of Hello World?
Expected: 100
Actual: 1
END EXECUTION - FAILED

Those are expected, I assume.

However there are failures that look more like real failures:

DeadCodeElimination+TestUnmodifiableInstanceFieldOptimization+WithInitOnlyPropertyWrite
System.Exception: Exception of type 'System.Exception' was thrown.
   at DeadCodeElimination.ThrowIfPresentWithUsableMethodTable(Type, String) + 0x6a
   at DeadCodeElimination.TestTypeOfCodegenBranchElimination.Run() + 0x109
   at DeadCodeElimination.Run() + 0x68
   at TrimmingBehaviors!<BaseAddress>+0x29bf43
   at Program.<<Main>$>g__RunTest|0_0(Func`1, String) + 0x64
===== Test DeadCodeElimination.Run failed =====

DeadCodeElimination+TestUnmodifiableInstanceFieldOptimization+WithInitOnlyPropertyWrite
System.Exception: Exception of type 'System.Exception' was thrown.
   at DeadCodeElimination.ThrowIfPresentWithUsableMethodTable(Type, String) + 0x6a
   at DeadCodeElimination.TestTypeOfCodegenBranchElimination.Run() + 0x109
   at DeadCodeElimination.Run() + 0x68
   at TrimmingBehaviors!<BaseAddress>+0x29bf43
   at Program.<<Main>$>g__RunTest|0_0(Func`1, String) + 0x64
===== Test DeadCodeElimination.Run failed =====

Is that another expectation from the JIT (deadcode elimination), that is not guaranteed by MinOpts?

MichalStrehovsky · 2024-11-28T06:13:28Z

Those three failures are due to the forced minopts since they all expect optimizations to happen. Smoketests tend to have tests that test optimizations happen. I think we should run nativeaot outerloop with the forced minopts and ignore all failures in tests that are only failing because of the minopts. There shouldn't be many more tests like that.

Unfortunately outerloop is on the floor right now, trying to fix it in #110238.

VSadov · 2024-11-28T16:21:42Z

Those three failures are due to the forced minopts since they all expect optimizations to happen

Just want to clarify - the failures are only because the tests explicitly check for optimizations (the first case that simply looks for size regressions looks like that). Or if optimizations do not happen the codegen is actually incorrect? I can't tell about ThrowIfPresentWithUsableMethodTable cases.

MichalStrehovsky · 2024-11-28T21:51:02Z

Those three failures are due to the forced minopts since they all expect optimizations to happen

Just want to clarify - the failures are only because the tests explicitly check for optimizations (the first case that simply looks for size regressions looks like that). Or if optimizations do not happen the codegen is actually incorrect? I can't tell about ThrowIfPresentWithUsableMethodTable cases.

The DeadCodeElimination tests test that branch removal happened and looks for that using side effects (it's hard to test branch removal directly). The side effect in this case is whether we have a full MethodTable for something.

I believe the lines:

    if (IsTargetAbi(CORINFO_NATIVEAOT_ABI))
    {
        theMinOptsValue = true;
    }

in this PR currently set minopts for everything (for testing purposes) so branch removal doesn't happen.

jakobbotsch · 2024-12-10T09:52:02Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-12-10T09:52:18Z

Azure Pipelines successfully started running 1 pipeline(s).

JIT: Avoid reporting call-sites in MinOpts on all targets

4c20c15

When the call-sites have no interesting GC information we avoid reporting them, but this behavior was enabled only for x86/x64. Enable this on other platforms too. Fix dotnet#103917

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 25, 2024

dotnet-policy-service bot assigned jakobbotsch Jun 25, 2024

jakobbotsch commented Jun 25, 2024

View reviewed changes

Run jit-format

fe57c11

build-analysis bot mentioned this pull request Jun 25, 2024

Nuget: Central Directory corrupt dotnet/dnceng#3099

Open

3 tasks

jakobbotsch added 2 commits June 25, 2024 14:31

Merge branch 'main' of github.com:dotnet/runtime into minopts-report-…

6b70654

…call-sites

No tracked GC slots even when prejitting MinOpts

6f9c8aa

VSadov mentioned this pull request Jun 25, 2024

[NativeAOT] Adding CET support #102680

Merged

build-analysis bot mentioned this pull request Jun 25, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

VSadov mentioned this pull request Jul 22, 2024

Revert "Do not encode safe points with -1 offset. (#104336)" #105234

Merged

jakobbotsch added this to the 10.0.0 milestone Aug 5, 2024

dotnet-policy-service bot removed this from the 10.0.0 milestone Sep 4, 2024

dotnet-policy-service bot closed this Sep 4, 2024

github-actions bot locked and limited conversation to collaborators Oct 6, 2024

jakobbotsch reopened this Oct 28, 2024

dotnet unlocked this conversation Oct 28, 2024

Merge branch 'main' into minopts-report-call-sites

550d837

build-analysis bot mentioned this pull request Oct 28, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

Merge branch 'main' into minopts-report-call-sites

110d72e

jakobbotsch marked this pull request as ready for review October 31, 2024 15:07

jakobbotsch requested review from jkotas and VSadov October 31, 2024 15:07

Force MinOpts for NAOT

4390b5c

jakobbotsch mentioned this pull request Nov 7, 2024

JIT: Mark some intrinsics as must-expand for NAOT due to ILScanner dependencies #109609

Merged

jakobbotsch added 2 commits November 11, 2024 17:12

Merge branch 'main' of github.com:dotnet/runtime into minopts-report-…

dcc02be

…call-sites

IsSafePoint -> CouldBeSafePoint

83fe9db

jakobbotsch requested a review from MichalStrehovsky as a code owner November 11, 2024 16:22

Fix in Unix version too

0a2738b

build-analysis bot mentioned this pull request Nov 11, 2024

"We stopped hearing from agent Azure Pipelines 32. Verify the agent machine is running and has a healthy network connection" dotnet/dnceng#1886

Open

3 tasks

Merge branch 'main' into minopts-report-call-sites

5408bf4

This was referenced Nov 27, 2024

The hosted runner encountered an error while running your job. (Error Type: Disconnect). dotnet/dnceng#1919

Open

[mono] Runtime tests LargeStructSize_Mono and xprecise1_cs_do failing on Apple mobile #110149

Open

Merge branch 'main' into minopts-report-call-sites

7a0577f

build-analysis bot mentioned this pull request Dec 10, 2024

Test failures in JSImportGenerator.Unit.Tests.Compiles.ValidateGeneratedSourceOutput #110575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Avoid reporting call-sites in MinOpts on all targets #103950

JIT: Avoid reporting call-sites in MinOpts on all targets #103950

jakobbotsch commented Jun 25, 2024

jakobbotsch Jun 25, 2024

jkotas Jun 25, 2024

VSadov Jun 25, 2024

jakobbotsch Jun 25, 2024

jakobbotsch commented Jun 25, 2024

azure-pipelines bot commented Jun 25, 2024

jakobbotsch commented Jul 9, 2024

azure-pipelines bot commented Jul 9, 2024

jakobbotsch commented Jul 10, 2024

azure-pipelines bot commented Jul 10, 2024

jakobbotsch commented Oct 31, 2024

VSadov commented Oct 31, 2024

jakobbotsch commented Nov 4, 2024

jkotas commented Nov 4, 2024

jakobbotsch commented Nov 5, 2024

jkotas commented Nov 5, 2024

MichalStrehovsky commented Nov 6, 2024

VSadov commented Nov 6, 2024

VSadov commented Nov 6, 2024 •

edited

Loading

jkotas commented Nov 6, 2024

jakobbotsch commented Nov 7, 2024 •

edited

Loading

jakobbotsch commented Nov 14, 2024

azure-pipelines bot commented Nov 14, 2024

VSadov commented Nov 28, 2024 •

edited

Loading

MichalStrehovsky commented Nov 28, 2024

VSadov commented Nov 28, 2024 •

edited

Loading

MichalStrehovsky commented Nov 28, 2024

jakobbotsch commented Dec 10, 2024

azure-pipelines bot commented Dec 10, 2024

JIT: Avoid reporting call-sites in MinOpts on all targets #103950

Are you sure you want to change the base?

JIT: Avoid reporting call-sites in MinOpts on all targets #103950

Conversation

jakobbotsch commented Jun 25, 2024

jakobbotsch Jun 25, 2024

Choose a reason for hiding this comment

jkotas Jun 25, 2024

Choose a reason for hiding this comment

VSadov Jun 25, 2024

Choose a reason for hiding this comment

jakobbotsch Jun 25, 2024

Choose a reason for hiding this comment

jakobbotsch commented Jun 25, 2024

azure-pipelines bot commented Jun 25, 2024

jakobbotsch commented Jul 9, 2024

azure-pipelines bot commented Jul 9, 2024

jakobbotsch commented Jul 10, 2024

azure-pipelines bot commented Jul 10, 2024

jakobbotsch commented Oct 31, 2024

VSadov commented Oct 31, 2024

jakobbotsch commented Nov 4, 2024

jkotas commented Nov 4, 2024

jakobbotsch commented Nov 5, 2024

jkotas commented Nov 5, 2024

MichalStrehovsky commented Nov 6, 2024

VSadov commented Nov 6, 2024

VSadov commented Nov 6, 2024 • edited Loading

jkotas commented Nov 6, 2024

jakobbotsch commented Nov 7, 2024 • edited Loading

jakobbotsch commented Nov 14, 2024

azure-pipelines bot commented Nov 14, 2024

VSadov commented Nov 28, 2024 • edited Loading

MichalStrehovsky commented Nov 28, 2024

VSadov commented Nov 28, 2024 • edited Loading

MichalStrehovsky commented Nov 28, 2024

jakobbotsch commented Dec 10, 2024

azure-pipelines bot commented Dec 10, 2024

VSadov commented Nov 6, 2024 •

edited

Loading

jakobbotsch commented Nov 7, 2024 •

edited

Loading

VSadov commented Nov 28, 2024 •

edited

Loading

VSadov commented Nov 28, 2024 •

edited

Loading