Optimizing a series of divisions (#74020) by SkiFoD · Pull Request #74314 · dotnet/runtime

SkiFoD · 2022-08-21T22:11:04Z

There was a cascade of DIVs/UDIVs generated for code snippets like this.
I tried to handle the overflow scenario like this
Now it folds into one DIV and the result of multiplication of parent and child values, if it is possible.

The same happens with MUL (example), but I decided to not touch it right now, because there is an optimization that changes MUL to Shifts. It complicates everything a little bit.

ghost · 2022-08-21T22:11:20Z

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

Issue Details

A draft to figure out if I'm on a right way.

Author:	SkiFoD
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

SkiFoD · 2022-08-23T05:57:42Z

/azp run runtime

azure-pipelines · 2022-08-23T05:57:47Z

Commenter does not have sufficient privileges for PR 74314 in repo dotnet/runtime

SkiFoD · 2022-08-24T07:17:56Z

@jakobbotsch Hey, could you please review this PR?

SkiFoD · 2022-09-03T06:43:37Z

@EgorBo Hey, could you please check the PR?

danmoseley · 2022-09-05T22:20:34Z

The same happens with MUL

@SkiFoD Do you want to open an issue for that too?

SkiFoD · 2022-09-11T12:20:44Z

The same happens with MUL

@SkiFoD Do you want to open an issue for that too?

@danmoseley Hey. Created an issue #75413 for the bug.

JulieLeeMSFT · 2022-10-03T20:50:08Z

@EgorBo, this community PR is ready for another review.

@EgorBo Hey, could you please check the PR?

SkiFoD · 2022-10-12T17:52:49Z

@EgorBo I updated the PR.

BruceForstall · 2022-10-15T00:08:53Z

@EgorBo PTAL

EgorBo · 2022-10-24T02:03:47Z

Oops, sorry for the delay, will review tomorrow

BruceForstall · 2022-12-12T16:53:28Z

@EgorBo PTAL

BruceForstall · 2022-12-19T18:38:16Z

@EgorBo ping

EgorBo · 2023-01-04T15:55:15Z


 #endif // defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64)

+#if defined(TARGET_XARCH)


Why it's limited to xarch only?

EgorBo · 2023-01-04T15:55:36Z

+                if (optimizedTree != nullptr)
+                {
+                    tree = optimizedTree;
+                    if (tree->IsIntegralConst())


it doesn't look like this condition can possibly be true?

EgorBo · 2023-01-04T15:56:05Z

+        // Make sure it is not folded into overflow
+        // If it is going to overflow, then result of such a division
+        // is 0
+        if ((upperBound / child_val) < root_val)


potential division by zero in jit code if child_val is 0

EgorBo · 2023-01-04T15:56:45Z

+        GenTree* chOp2 = op1->gtGetOp2();
+
+        IntegralRange boundRange = IntegralRange::ForNode(op2, this);
+        int64_t       upperBound = IntegralRange::SymbolicToRealValue(boundRange.GetUpperBound());


it's not clear to me why IntegralRange is used here - you already have child_val and root_val, don't you?

EgorBo · 2023-01-04T15:58:47Z

From what I see locally this PR only hits a single diff across all collections so it needs unit tests at least, e.g. the division by zero case. e.g. this code:

uint foo() => 0;

uint ff(uint u2) => u2 / foo() / 4;

crashes jit in this PR

JulieLeeMSFT · 2023-02-13T17:50:37Z

CC @TIHan will look into this.

TIHan

Overall, this looks fine. There is an outstanding @EgorBo comment regarding the IntegralRange and why we need it.

I would suggest that we add a few disasm check tests, see here for details on how to do this.

ghost · 2023-03-01T03:02:02Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

SkiFoD · 2023-03-06T07:03:10Z

Overall, this looks fine. There is an outstanding @EgorBo comment regarding the IntegralRange and why we need it.

I would suggest that we add a few disasm check tests, see here for details on how to do this.

The code changes produce too small diffs, so I'm not sure if it is worth it to keep the code or it is just better to reject the PR.

EgorBo · 2023-03-27T07:40:08Z

Let's focus on a more impactful optimization (in DM)

ghost added community-contribution Indicates that the PR has been added by a community member area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Aug 21, 2022

SkiFoD force-pushed the skifod/issue-74020 branch from 931f18c to 90a110a Compare August 22, 2022 19:51

SkiFoD changed the title ~~Draft of optimizing a series of divisions (#74020)~~ Optimizing a series of divisions (#74020) Aug 24, 2022

SkiFoD marked this pull request as ready for review August 24, 2022 07:16

EgorBo reviewed Aug 24, 2022

View reviewed changes

Comment thread src/coreclr/jit/morph.cpp Resolved

EgorBo reviewed Aug 24, 2022

View reviewed changes

Comment thread src/coreclr/jit/morph.cpp Resolved

JulieLeeMSFT added this to the 8.0.0 milestone Aug 24, 2022

SkiFoD requested a review from EgorBo August 30, 2022 06:11

JulieLeeMSFT assigned EgorBo and SkiFoD Sep 12, 2022

SkiFoD added 4 commits October 12, 2022 19:46

Draft of optimizing a series of divisions (dotnet#74020)

8760304

Potential fix of divided by zero error (dotnet#74020)

257e1c6

Division by overflowed values results in zero (dotnet#74020)

87ee2ec

Removed VNs presaving

03f5404

SkiFoD force-pushed the skifod/issue-74020 branch from cc2f94f to 03f5404 Compare October 12, 2022 17:47

This was referenced Oct 12, 2022

System.Data.OleDb.Test - timeout and hangs #74488

Closed

Nuget restore error caused by 503s dotnet/arcade#11239

Closed

Merge branch 'main' of github.com:dotnet/runtime into skifod/issue-74020

6d1279b

EgorBo reviewed Jan 4, 2023

View reviewed changes

SkiFoD added 2 commits January 14, 2023 11:24

Merge branch 'main' into skifod/issue-74020

3708cd3

Fix for division by zero (dotnet#75413)

3ed0ac5

build-analysis Bot mentioned this pull request Jan 15, 2023

tracing/eventpipe/eventsourceerror/eventsourceerror/eventsourceerror failure #80666

Closed

runfoapp Bot mentioned this pull request Jan 16, 2023

Infra improvements for Helix #68176

Closed

Moved the logic to fgOptimizeCommutativeArithmetic (dotnet#75413)

13a7e54

SkiFoD force-pushed the skifod/issue-74020 branch from 75b6855 to 13a7e54 Compare January 22, 2023 18:19

JulieLeeMSFT requested a review from TIHan February 13, 2023 17:50

TIHan suggested changes Feb 14, 2023

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 14, 2023

ghost added the no-recent-activity label Mar 1, 2023

ghost removed needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity labels Mar 6, 2023

EgorBo closed this Mar 27, 2023

ghost locked as resolved and limited conversation to collaborators Apr 26, 2023


		#endif // defined(TARGET_ARM64) \|\| defined(TARGET_LOONGARCH64)

		#if defined(TARGET_XARCH)

Conversation

SkiFoD commented Aug 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 21, 2022

Uh oh!

SkiFoD commented Aug 23, 2022

Uh oh!

azure-pipelines Bot commented Aug 23, 2022

Uh oh!

SkiFoD commented Aug 24, 2022

Uh oh!

Uh oh!

Uh oh!

SkiFoD commented Sep 3, 2022

Uh oh!

danmoseley commented Sep 5, 2022

Uh oh!

SkiFoD commented Sep 11, 2022

Uh oh!

JulieLeeMSFT commented Oct 3, 2022

Uh oh!

SkiFoD commented Oct 12, 2022

Uh oh!

BruceForstall commented Oct 15, 2022

Uh oh!

EgorBo commented Oct 24, 2022

Uh oh!

BruceForstall commented Dec 12, 2022

Uh oh!

BruceForstall commented Dec 19, 2022

Uh oh!

EgorBo Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

EgorBo Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

EgorBo Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

EgorBo Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Jan 4, 2023

Uh oh!

JulieLeeMSFT commented Feb 13, 2023

Uh oh!

TIHan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 1, 2023

Uh oh!

SkiFoD commented Mar 6, 2023

Uh oh!

EgorBo commented Mar 27, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SkiFoD commented Aug 21, 2022 •

edited

Loading

TIHan left a comment •

edited

Loading