Skip to content

Emit == null instead of <= null for niche check #74533

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

Merged
merged 1 commit into from
Aug 8, 2020

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Jul 19, 2020

When the niche maximum is zero, emit a "== zero" check instead of a "<= zero" check. In particular, this avoids the awkward case of "<= null". While LLVM does canonicalize this to "== null", this apparently doesn't happen for constant expressions, leading to the issue in #74425. While that can be addressed on the LLVM side, it still seems prudent to emit sensible IR here, because this will allow null checks to be optimized earlier in the pipeline.

Fixes #74425.

@rust-highfive
Copy link
Contributor

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2020
@Mark-Simulacrum
Copy link
Member

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned petrochenkov Jul 19, 2020
@joshtriplett
Copy link
Member

This looks great to me, including the added test.

@joshtriplett joshtriplett added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 20, 2020
@Muirrum Muirrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2020
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

r=me with or without the comment resolved

When the niche maximum is zero, emit a "== zero" check instead of
a "<= zero" check. In particular, this avoid the awkward case of
"<= null". While LLVM does canonicalize this to "!= null", this
appently doesn't happen for constant expressions, leading to the
issue in rust-lang#74425. While that can be addressed on the LLVM side, it
still seems prudent to emit sensible IR here, because this will
allow null checks to be optimized earlier in the pipeline.

Fixes rust-lang#74425.
@nikic
Copy link
Contributor Author

nikic commented Aug 8, 2020

@bors r=eddyb

@bors
Copy link
Collaborator

bors commented Aug 8, 2020

📌 Commit 7e5c7cf has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2020
@bors
Copy link
Collaborator

bors commented Aug 8, 2020

⌛ Testing commit 7e5c7cf with merge 2bac92b...

@bors
Copy link
Collaborator

bors commented Aug 8, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing 2bac92b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 8, 2020
@bors bors merged commit 2bac92b into rust-lang:master Aug 8, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missed optimization: The optimizer forgets that Some(function) is Some(_)
9 participants