Skip to content

Remove potential NULLCHECK creation after LIR on arm32. #40349

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.

Sign up for GitHub

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

Jump to bottom
Merged
sandreenko merged 3 commits into from
Aug 5, 2020
Merged

Remove potential NULLCHECK creation after LIR on arm32. #40349

sandreenko merged 3 commits into from
Aug 5, 2020

Conversation

@sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Aug 4, 2020

@erozenfeld noticed that changes from #39824 could work wrong on arm32 because we could create a NullCheck node after Lower phase. If it reaches codegen it will fail there. I don't have a repro test, this path is probably unreachable for arm32 currently, but it is better to use a newly added function from #40226 to handle it.

Also, use TYP_INT for the new NULLCHECK nodes, before in some places we were using TYP_BYTE. The actual type doesn't matter, codegen ignores it.

No libs diffs.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 4, 2020
@sandreenko
Copy link
Contributor Author

sandreenko commented Aug 4, 2020

PTAL @erozenfeld @CarolEidt @dotnet/jit-contrib

erozenfeld
erozenfeld approved these changes Aug 4, 2020
View reviewed changes
Copy link
Member

@erozenfeld erozenfeld left a comment

Choose a reason for hiding this comment

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

LGTM

CarolEidt
CarolEidt approved these changes Aug 4, 2020
View reviewed changes
Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM, though I believe that historically GT_NULLCHECKs have always been TYP_BYTE (not sure why, since codegen ignores it) so I'm just curious why you changed it.

@sandreenko
Copy link
Contributor Author

sandreenko commented Aug 4, 2020

LGTM, though I believe that historically GT_NULLCHECKs have always been TYP_BYTE (not sure why, since codegen ignores it) so I'm just curious why you changed it.

We are changing it to TYP_INT in TransformUnusedIndirection (and before that we were changing it to TYP_INT in LowerIndir, https://github.com/dotnet/runtime/pull/40226/files). So it was a little confusing that we were changing it once to TYP_INT, then calling gtChangeOperToNullCheck and changing it to TYP_BYTE.

I could make a change in TransformUnusedIndirection and use TYP_BYTE there instead.

@CarolEidt
Copy link
Contributor

CarolEidt commented Aug 5, 2020

I could make a change in TransformUnusedIndirection and use TYP_BYTE there instead.

No need; I was just curious about the reasoning. Thanks

@sandreenko sandreenko merged commit 02db286 into dotnet:master Aug 5, 2020
@sandreenko sandreenko deleted the fixPotentialNullCheckCreationAfterLIRForArm32 branch August 5, 2020 04:17
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@Jacksondr5
Remove potential NULLCHECK creation after LIR on arm32. (dotnet#40349)
157f195 
* Use `TransformUnusedIndirection` in LIR liveness.

* Use `TYP_INT` as `TransformUnusedIndirection` does.

* fix grammar.
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@erozenfeld erozenfeld erozenfeld approved these changes

+1 more reviewer

@CarolEidt CarolEidt CarolEidt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@sandreenko @CarolEidt @erozenfeld @karelz