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

[JIT] ARM64 - Fix Assertion failed 'node->OperIs(GT_DIV, GT_UDIV, GT_MOD)' during 'Lowering nodeinfo' #85664

Merged
merged 4 commits into from
May 3, 2023

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented May 2, 2023

Resolves #85630

Description

There was a case where the node was lowered from LowerModPow2 but node->gtNext would return nullptr from LowerConstIntDivOrMod which indicates the node was not transformed when it actually was.

Best way to solve this is return a bool indicating if the transformation was successful or not and also provide an out parameter for the next node to lower. So, this renames LowerConstIntDivOrMod to TryLowerConstIntDivOrMod with an out parameter for the next node to lower.

@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 May 2, 2023
@ghost ghost assigned TIHan May 2, 2023
@ghost
Copy link

ghost commented May 2, 2023

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

Issue Details

Resolves #85630

Description

There was a case where the node was lowered from LowerModPow2 but node->gtNext would return nullptr from LowerConstIntDivOrMod which indicates the node was not transformed when it actually was.

Best way to solve this is return a bool indicating if the transformation was successful or not, and also provide an out parameter for the next node to lower. So, this renames LowerConstIntDivOrMod to TryLowerConstIntDivOrMod with an out parameter for the next node to lower.

Author: TIHan
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@TIHan
Copy link
Contributor Author

TIHan commented May 2, 2023

@dotnet/jit-contrib @kunalspathak this is ready - no diffs

Current CI failure is unrelated.

{
assert((node->OperGet() == GT_DIV) || (node->OperGet() == GT_MOD));
assert(nextNode != nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be initialized *nextNode = nullptr which should be the case if this method returns false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a similar TryLowerAndNegativeOne with an out parameter nextNode and we do not set that to nullptr or when we return false.

{
return newNode;
return nextNode;
Copy link
Member

Choose a reason for hiding this comment

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

assert(nextNode != nullptr)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that nextCode can be a nullptr even if the transformation was successful.

But, we could add an assert assert(nextNode == nullptr); if the transformation wasn't successful.

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels May 2, 2023
@TIHan
Copy link
Contributor Author

TIHan commented May 2, 2023

@kunalspathak this is ready again

@kunalspathak
Copy link
Member

Surprisingly the simplified test is still not simple, but don't want to waste your time trying to trim it further.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@TIHan
Copy link
Contributor Author

TIHan commented May 3, 2023

Merging. CI failure is unrelated.

@TIHan TIHan merged commit 56eff9f into dotnet:main May 3, 2023
@TIHan TIHan deleted the 85630-fix branch May 3, 2023 03:06
@ghost ghost locked as resolved and limited conversation to collaborators Jun 2, 2023
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.

Assertion failed 'node->OperIs(GT_DIV, GT_UDIV, GT_MOD)' during 'Lowering nodeinfo'
3 participants