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

Update the assert to correctly check if a tree with value has non-zero destination registers #81412

Merged
merged 6 commits into from
Feb 3, 2023

Conversation

kunalspathak
Copy link
Member

In #77078, we based an indir to NOP and in MinOpts, since we do not perform liveness, it stays until LSRA. In LSRA, during building the nodes, we should be checking if a tree has a value, the number of destination registers should be non-zero. Instead, we were checking if tree has unused value, it should have non-zero destination registers, which was not making sense. Updated the assert.

Fixes: #81356

@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 Jan 31, 2023
@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib

@ghost ghost assigned kunalspathak Jan 31, 2023
@ghost
Copy link

ghost commented Jan 31, 2023

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

Issue Details

In #77078, we based an indir to NOP and in MinOpts, since we do not perform liveness, it stays until LSRA. In LSRA, during building the nodes, we should be checking if a tree has a value, the number of destination registers should be non-zero. Instead, we were checking if tree has unused value, it should have non-zero destination registers, which was not making sense. Updated the assert.

Fixes: #81356

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

Can we remove the node in lowering instead of creating a NOP? All other places in lowering do BlockRange().Remove(node) instead of bashing to a NOP.

It's also odd to me that GenTree::IsUnusedValue() returns true for the NOP here, given that it isn't even a value.

@kunalspathak
Copy link
Member Author

Can we remove the node in lowering instead of creating a NOP? All other places in lowering do BlockRange().Remove(node) instead of bashing to a NOP.

Ah, my impression was that this would have got removed with liveness update after lowering, but didn't realize I could just delete it. The fix in lsra is still needed though because that assert was wrong.

It's also odd to me that GenTree::IsUnusedValue() returns true for the NOP here, given that it isn't even a value.

I am not sure about it, but we do call TransformUnusedIndirection() if unused value but then there is a gap of not clearing the value?

@JulieLeeMSFT
Copy link
Member

Thanks @kunalspathak for this fix.

@kunalspathak kunalspathak merged commit 3ced968 into dotnet:main Feb 3, 2023
@kunalspathak kunalspathak deleted the issue_81356 branch February 3, 2023 19:50
@ghost ghost locked as resolved and limited conversation to collaborators Mar 6, 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.

JIT: Assertion failed '!tree->IsUnusedValue() || (dstCount != 0)' during 'Linear scan register alloc'
3 participants