[JIT] ARM64 - Overflow check optimizations for division #82924
Conversation
…tant that is a not a min value
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsWill resolve #46010 At the moment, I haven't got rid of the redundant branch operations yet.
|
TIHan
changed the title
TIHan
changed the title
|
@dotnet/jit-contrib @kunalspathak This is ready. Straight-forward change, should be an easy review. Diffs |
src/coreclr/jit/morph.cpp
Outdated
| } | ||
| } | ||
|
|
||
| if (checkDividend) |
There was a problem hiding this comment.
NOTE: this still leaves overflow block alive if it's unused but presumably it's a different issue
There was a problem hiding this comment.
Hhm, won't it be dead code eliminated?
There was a problem hiding this comment.
Throw helper blocks are marked as no-remove. This is because they aren't rooted in the normal flowgraph (the edges from the throwing nodes to the throw helper blocks are implicit).
There was a problem hiding this comment.
Hhm, won't it be dead code eliminated?
these blocks are special, they have BBF_DONT_REMOVE or something like that. We spawn them early in importer/morph and then tie to corresponding divisions if needed, it's too late to remove them in codegen
There was a problem hiding this comment.
I'm hopeful that this PR will resolve those cases as it's already happening today.
| // Return Value: | ||
| // true if the given tree is known to never be negative one | ||
| // | ||
| bool GenTree::IsNeverNegativeOne(Compiler* comp) const |
There was a problem hiding this comment.
this helper is only needed for division in one place it seems?
There was a problem hiding this comment.
It's only in one place, but it does help make things clean.
ghost
added
the
needs-author-action
ghost
removed
the
needs-author-action
|
@dotnet/jit-contrib @BruceForstall This is ready again. Diffs |
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
…Flags. Use GetExceptionSetFlags to determine to add div-by-zero or overflow blocks for arm64.
|
Thank you all for the feedback. The PR feels a lot better. Since @SingleAccretion pointed me to I wonder if there are any more duplicate checks. |
|
@dotnet/jit-contrib ready again. Got more wins in full-opts compared to before: diffs |
|
/azp run runtime-coreclr jitstress gcstress |
|
No pipelines are associated with this pull request. |
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr gcstress |
|
No pipelines are associated with this pull request. |
|
/azp run fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@jakobbotsch - I think it's fine that we do this optimization in min-opts, we were already doing it before. It's not going to hurt the debugging experience for users as its all generated internally; I don't see how this would impact anyone. |
Description
Resolves #46010
This PR will not emit an arithmetic exception block if we know the dividend of a division operation is constant and not a MinValue.
This PR will also shorten the emitted code when testing for an overflow of a division operation.
Acceptance Criteria