Slightly improve MinOpts JIT TP #105250

EgorBo · 2024-07-22T14:51:54Z

A few tricks to mitigate regressions which raised concerns in #105190

PS: I still want to rewrite all OptimizationEnabled/Disabled into an OptLevel like Andy once suggested, but didn't want to do it here.

dotnet-policy-service · 2024-07-22T14:52:32Z

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

tannergooding · 2024-07-23T01:54:59Z

src/coreclr/jit/compiler.h

+            assert(compMinOptsIsSet);
+            return canUseAllOpts;
+        }
+        bool Tier0OptimizationEnabled() const


A minor nit is that OptimizationEnabled and Tier0OptimizationEnabled both existing may be a bit confusing. Is there any idea what we would call the levels if this were some kind of OptimizationLevel enum, just for ideas on what we could call it as an alternative?

I think this is fine for now, because getting better names is a more involved change; just wanted to check if there were any other ideas/suggestions for the interim

Yep, I agree that it doesn't look pretty, but a proper refactoring will be quite big and hard to review

any idea what we would call the levels if this were some kind of OptimizationLevel enum, just for ideas on what we could call it as an alternative?

Not yet. Andy suggested to use OptLevel: NoOpts, Size, Speed where Size implies "light-weight". The problem that we already have optLevel in jit, but we never use it (it's always Blended) so it all needs some re-thinking

src/coreclr/jit/gentree.cpp

src/coreclr/jit/vartype.h

tannergooding · 2024-07-23T02:01:10Z

src/coreclr/jit/gentree.cpp

+    }
+
+    unsigned level = 1;
+    if (tree->OperIsSimple())


nit: Might be worth a small comment saying that we are intentionally not handling hwintrinsics (they otherwise normally support GTF_REVERSE_OPS and other considerations).

I'll add a comment, but we skip a lot of trees here, it was driven by asmdiff + tpdiff results

but we skip a lot of trees here

👍, had mostly thought it might be worth a callout because it normally mirrors a lot of the SimpleOp handling (unlike any other node), so figured it might be worth a special note/callout

tannergooding

This LGTM and is a very nice improvement overall.

Couple questions and one missed place where the new logic could also be used.

EgorBo · 2024-07-23T20:03:03Z

@jakobbotsch @tannergooding cc @dotnet/jit-contrib Could you please take another look as I've pushed more changes since your last approve.

TL;DR: I've introduced a stripped version of gtSetEvalOrder for MinOpts: gtSetEvalOrderMinOpts it doesn't calculate costs at all, so only operand swapping is preserved for certain operations. It contains a few quirks to minimize size regression impact.

As we discussed in Discord, it might makes sense to introduce a single IR walk e.g. before Rationalization to do the swap where profitable, but I am leaving this for a future PR.

Diffs with noticeable improvements for MinOpts as expected.

jakobbotsch · 2024-07-23T20:14:55Z

src/coreclr/jit/gentree.cpp

+        // We don't re-order operands in LIR anyway.
+        return 0;


Does it ever get called?

Does it ever get called?

Not sure, but the original function gtSetEvalOrder contained this check

jakobbotsch · 2024-07-23T20:16:03Z

src/coreclr/jit/gentree.cpp

+        // Check for a nilary operator
+        if (op1 == nullptr)
+        {
+            assert(op2 == nullptr);


In what cases do we get here?

it's also copied from the original function. Looks like GT_RETURN for void is so (and RETFILT)

jakobbotsch · 2024-07-23T20:17:31Z

src/coreclr/jit/gentree.cpp

+        unsigned levelOp2 = gtSetEvalOrderMinOpts(op2);
+
+        bool allowSwap = true;
+        // TODO: Introduce a function to check whether we can swap the order of its operands or not.


Should this TODO be removed? Not sure I understand what it refers to. If it refers to extract the below logic into a function, why not just do that in this PR?

I'd better remove the comment then for now, I don't mind. The idea is to share quirks with the Tier1 version (esp for STOREIND/STORE_BLK)

Might be useful enough to leave a comment that it should be kept in sync with the quirks in the tier 1 version (and likewise in the tier 1 version)

Moved both to a single helper, to keep the quirks in one place

jakobbotsch · 2024-07-23T20:19:50Z

src/coreclr/jit/gentree.cpp

+                }
+
+                // In case op2 assigns to a local var that is used in op1, we have to evaluate op1 first.
+                if (gtMayHaveStoreInterference(op2, op1))


You probably don't need this expensive version in MinOpts. It can be the conservative (op2->gtFlags & GTF_ASG) != 0 that existed before #97409.

You probably don't need this expensive version in MinOpts. It can be the conservative (op2->gtFlags & GTF_ASG) != 0 that existed before #97409.

just tried - it adds +28k bytes size regression for benchmarks.run_pgo collection

Ah ok, fine to leave it then. (Actually looks like I called out explicitly doing it in MinOpts in that PR as well)

Slightly improve MinOpts JIT TP

a44cb1a

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

dotnet-policy-service bot assigned EgorBo Jul 22, 2024

EgorBo mentioned this pull request Jul 22, 2024

Call gtFoldExpr in morph for tier0 #105190

Merged

Update morph.cpp

62a78e8

This was referenced Jul 22, 2024

'System.Net.NameResolution.Functional.Tests' Failure #105092

Closed

linux-armel checked CoreCLR_NonPortable failing to build with "Unable to find toolchain executable. Name: 'ar', Prefix: 'llvm-'" #105176

Closed

jakobbotsch approved these changes Jul 22, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jul 22, 2024

System.IO.Tests.Directory_Create_Tests.FileSystemWatcher_Directory_Create_InNestedDirectory fails with "Created event did not occur as expected" #103630

Open

EgorBo added 3 commits July 23, 2024 02:20

Introduce gtSetEvalOrderMinOpts

f76354d

fix ci

28e67dd

Update morph.cpp

7ffac16

tannergooding reviewed Jul 23, 2024

View reviewed changes

src/coreclr/jit/gentree.cpp Show resolved Hide resolved

tannergooding reviewed Jul 23, 2024

View reviewed changes

src/coreclr/jit/vartype.h Show resolved Hide resolved

tannergooding reviewed Jul 23, 2024

View reviewed changes

tannergooding approved these changes Jul 23, 2024

View reviewed changes

This was referenced Jul 23, 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

Test failure: GC\\Features\\HeapExpansion\\Finalizer\\Finalizer.cmd #102706

Closed

Clean up

afb150f

EgorBo added 2 commits July 23, 2024 18:34

test

7a2a7ac

Clean up

029a747

tannergooding approved these changes Jul 23, 2024

View reviewed changes

jakobbotsch reviewed Jul 23, 2024

View reviewed changes

EgorBo added 2 commits July 23, 2024 23:02

clean up

dc38df2

fix build

93dcf59

This was referenced Jul 24, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Open

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

jakobbotsch approved these changes Jul 24, 2024

View reviewed changes

EgorBo merged commit 5e1081f into dotnet:main Jul 24, 2024
108 checks passed

EgorBo deleted the fix-tp-regressions branch July 24, 2024 11:51

LoopedBard3 mentioned this pull request Jul 30, 2024

[Perf] Linux/x64: 1 Regression on 7/24/2024 2:56:52 PM dotnet/perf-autofiling-issues#39086

Open

github-actions bot locked and limited conversation to collaborators Aug 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slightly improve MinOpts JIT TP #105250

Slightly improve MinOpts JIT TP #105250

EgorBo commented Jul 22, 2024 •

edited

Loading

dotnet-policy-service bot commented Jul 22, 2024

tannergooding Jul 23, 2024

EgorBo Jul 23, 2024

EgorBo Jul 23, 2024

tannergooding Jul 23, 2024

EgorBo Jul 23, 2024

tannergooding Jul 23, 2024

tannergooding left a comment

EgorBo commented Jul 23, 2024

jakobbotsch Jul 23, 2024

EgorBo Jul 23, 2024

jakobbotsch Jul 23, 2024

EgorBo Jul 23, 2024 •

edited

Loading

jakobbotsch Jul 23, 2024

EgorBo Jul 23, 2024

jakobbotsch Jul 23, 2024

EgorBo Jul 23, 2024

jakobbotsch Jul 23, 2024

EgorBo Jul 23, 2024

jakobbotsch Jul 23, 2024

Slightly improve MinOpts JIT TP #105250

Slightly improve MinOpts JIT TP #105250

Conversation

EgorBo commented Jul 22, 2024 • edited Loading

dotnet-policy-service bot commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding left a comment

Choose a reason for hiding this comment

EgorBo commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo commented Jul 22, 2024 •

edited

Loading

EgorBo Jul 23, 2024 •

edited

Loading