Skip to content

ARM64 - Do not emit possible DIV_BY_ZERO/OVERFLOW exception blocks for non-zero/non-(negative one) constants - #68945

Merged
TIHan merged 6 commits into
dotnet:mainfrom
TIHan:div-by-zero-check
May 18, 2022
Merged

ARM64 - Do not emit possible DIV_BY_ZERO/OVERFLOW exception blocks for non-zero/non-(negative one) constants#68945
TIHan merged 6 commits into
dotnet:mainfrom
TIHan:div-by-zero-check

Conversation

@TIHan

@TIHanTIHan commented May 6, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Description

I noticed that we do not have to emit DIV_BY_ZERO/OVERFLOW exceptions if we know the second operand is a non-zero/non-(negative) integer constant respectively.

Acceptance Criteria

  • CI passes

@ghostghost assigned TIHanMay 6, 2022
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 6, 2022
@ghost

ghost commented May 6, 2022

Copy link
Copy Markdown

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

Issue Details

Description
I noticed that we do not have to emit DIV_BY_ZERO exceptions if we know the second operand is a non-zero integer constant.

Acceptance Criteria

  • CI passes
Author:TIHan
Assignees:TIHan
Labels:

area-CodeGen-coreclr

Milestone:-

jkotas
jkotas reviewed May 6, 2022
View reviewed changes
Comment threadsrc/coreclr/jit/morph.cpp Outdated
@TIHanTIHan changed the title Do not emit possible DIV_BY_ZERO exception for non-zero constantsDo not emit possible DIV_BY_ZERO/OVERFLOW exception blocks for non-zero/non-(negative one) constantsMay 6, 2022
@TIHanTIHan changed the title Do not emit possible DIV_BY_ZERO/OVERFLOW exception blocks for non-zero/non-(negative one) constantsARM64 - Do not emit possible DIV_BY_ZERO/OVERFLOW exception blocks for non-zero/non-(negative one) constantsMay 6, 2022
@TIHan

TIHan commented May 11, 2022

Copy link
Copy Markdown
ContributorAuthor

@dotnet/jit-contrib PR is ready. I don't think there is anything else to be done here.

BruceForstall
BruceForstall approved these changes May 12, 2022
View reviewed changes

@BruceForstallBruceForstall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does the -1 case show up in diffs? If not, can you add a test case?

@TIHan

TIHan commented May 12, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Does the -1 case show up in diffs? If not, can you add a test case?

If the constant was -1, we shouldn't see a diff as the overflow block will still be emitted.

What we will see is the regression diffs from this: #68885 (comment)
will be much less and the overflow check won't be emitted.

@TIHan

TIHan commented May 12, 2022

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-coreclr

@azure-pipelines

azure-pipelinesBot commented May 12, 2022

Copy link
Copy Markdown
No pipelines are associated with this pull request.

kunalspathak
kunalspathak approved these changes May 17, 2022
View reviewed changes

@kunalspathakkunalspathak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@TIHan
TIHan merged commit 6425a15 into dotnet:mainMay 18, 2022
@ghostghost locked as resolved and limited conversation to collaborators Jun 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas left review comments
+2 more reviewers
@BruceForstallBruceForstallBruceForstall approved these changes
@kunalspathakkunalspathakkunalspathak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@TIHanTIHan

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@TIHan@jkotas@BruceForstall@kunalspathak