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

Call gtFoldExpr in morph for tier0 #105190

Merged
merged 2 commits into from
Jul 22, 2024
Merged

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jul 20, 2024

gtFoldExpr doesn't need to be wrapped with opts.OptimizationEnabled() since it already checks it inside and allows some lightweight optimizations for tier0 (but not for minopts and/or debug).

Seeing some nice tier0 diffs from it.

@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 Jul 20, 2024
Copy link
Contributor

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

tree = gtFoldExpr(tree);
}
// Try to fold it, maybe we get lucky,
tree = gtFoldExpr(tree);
Copy link
Member

Choose a reason for hiding this comment

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

We have a similar check for hwintrinsic, right? Should that be included in this or as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, looks like those are fine as is

@EgorBo EgorBo merged commit 27f52d9 into dotnet:main Jul 22, 2024
104 of 108 checks passed
@EgorBo EgorBo deleted the call-gtfoldexpr-tier0 branch July 22, 2024 12:18
@jakobbotsch
Copy link
Member

Is the TP impact worth it?

@EgorBo
Copy link
Member Author

EgorBo commented Jul 22, 2024

Is the TP impact worth it?

are 0.1%-0.2% for real run collections a big deal? We currently have a problem when hot Tier0 code can stuck for 10ish seconds in tier0 due to promotion-queue problem in large apps leading to poor RPS till it reaches the steady state. Initially I also did a small clean up in OptimizationEnabled/OptimizationDisabled that improved TP by 0.15% just didn't push as part of this pr

@jakobbotsch
Copy link
Member

Is the TP impact worth it?

are 0.1%-0.2% for real run collections a big deal? We currently have a problem when hot Tier0 code can stuck for 10ish seconds in tier0 due to promotion-queue problem in large apps leading to poor RPS till it reaches the steady state. Initially I also did a small clean up in OptimizationEnabled/OptimizationDisabled that improved TP by 0.15% just didn't push as part of this pr

For me it's more about that we (the JIT team) should align on philosophy of the trade-offs between CQ and TP in tier-0. We can certainly live with such small TP impact (and I wouldn't bother reverting this or anything), but in my opinion tier-0 changes need additional motivation than just CQ if they are trading off for TP. E.g. this change trades off +0.14% TP for -0.06% code size in benchmarks.run_pgo for the tier-0 contexts -- I personally wouldn't have gone for that.

@jakobbotsch
Copy link
Member

Also cc @dotnet/jit-contrib for opinions.

@EgorBo
Copy link
Member Author

EgorBo commented Jul 22, 2024

To be fair, we can't even detect any visible improvements from changes like -10.0% in MinOpts in any of our benchmarks (TE startup time/time to first request). I think it's more important for Tier0 to call certain JIT-EE APIs less (because some of them may have locks, may trigger type loading etc), but we currently have no infra to measure that.

@jakobbotsch
Copy link
Member

jakobbotsch commented Jul 22, 2024

To be fair, we can't even detect any visible improvements from changes like -10.0% in MinOpts in any of our benchmarks (TE startup time/time to first request).

Well, to be fair we have never done any proper study of this. The variance on this number is just so high that you need hundreds or thousands of runs on the commits before and after the change to be able to detect the JIT change in the output (couple that with the fact that large chunk of start up time is also not spent in JIT).
I definitely agree that 10% in JIT does not translate to 10% in start up time, and maybe it's easier to make up for JIT regressions in other places.

I think it's more important for Tier0 to call certain JIT-EE APIs less (because some of them may have locks, may trigger type loading etc), but we currently have no infra to measure that.

I agree -- that would be one of the "additional motivations" we can have for changes to tier-0 codegen.

@EgorBo
Copy link
Member Author

EgorBo commented Jul 22, 2024

Filed #105250 - it's zero asm diffs with up to -0.5% TP improvements for MinOpts

@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2024
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.

3 participants