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

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).

…call-sites

jakobbotsch · 2025-01-07T14:18:55Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2025-01-07T14:19:14Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2025-01-08T12:00:27Z

I went through all the failures and I do not see any suspicious ones; they all look to be expected failures from turning off optimizations.

This reverts commit 4390b5c.

jkotas

Thanks!

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

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

Closed

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

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

dea14a4

…call-sites

build-analysis bot mentioned this pull request Jan 6, 2025

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

3 tasks

Revert "Force MinOpts for NAOT"

b7aa263

This reverts commit 4390b5c.

jkotas approved these changes Jan 8, 2025

View reviewed changes

VSadov approved these changes Jan 8, 2025

View reviewed changes

jakobbotsch merged commit 058fe35 into dotnet:main Jan 9, 2025
106 of 108 checks passed

jakobbotsch deleted the minopts-report-call-sites branch January 9, 2025 09:05

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

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

jakobbotsch commented Jan 7, 2025

azure-pipelines bot commented Jan 7, 2025

jakobbotsch commented Jan 8, 2025

jkotas left a comment

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

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

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

jakobbotsch commented Jan 7, 2025

azure-pipelines bot commented Jan 7, 2025

jakobbotsch commented Jan 8, 2025

jkotas left a comment

Choose a reason for hiding this comment

jakobbotsch commented Nov 7, 2024 •

edited

Loading

VSadov commented Nov 28, 2024 •

edited

Loading

VSadov commented Nov 28, 2024 •

edited

Loading