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

Codegen backend handles compare binops for the unit type #63551

Closed
bjorn3 opened this issue Aug 14, 2019 · 8 comments · Fixed by #65605
Closed

Codegen backend handles compare binops for the unit type #63551

bjorn3 opened this issue Aug 14, 2019 · 8 comments · Fixed by #65605
Labels
A-codegen Area: Code generation C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bjorn3
Copy link
Member

bjorn3 commented Aug 14, 2019

mir::BinOp::Ne | mir::BinOp::Lt | mir::BinOp::Gt |
mir::BinOp::Eq | mir::BinOp::Le | mir::BinOp::Ge => if is_unit {
bx.cx().const_bool(match op {
mir::BinOp::Ne | mir::BinOp::Lt | mir::BinOp::Gt => false,
mir::BinOp::Eq | mir::BinOp::Le | mir::BinOp::Ge => true,
_ => unreachable!()
})

libcore however doesn't use those for the implementations for PartialEq and PartialOrd for the unit type.

rust/src/libcore/cmp.rs

Lines 933 to 939 in c43d03a

#[stable(feature = "rust1", since = "1.0.0")]
impl PartialEq for () {
#[inline]
fn eq(&self, _other: &()) -> bool { true }
#[inline]
fn ne(&self, _other: &()) -> bool { false }
}

rust/src/libcore/cmp.rs

Lines 980 to 985 in c43d03a

impl PartialOrd for () {
#[inline]
fn partial_cmp(&self, _: &()) -> Option<Ordering> {
Some(Equal)
}
}

This means that the special case in cg_ssa can be removed.

@jonas-schievink jonas-schievink added A-codegen Area: Code generation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 14, 2019
@Centril Centril added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Aug 14, 2019
@punitkoura
Copy link

Hi @bjorn3 @Centril, I'd like to work on this. Would be a good initial contribution to Rust :)

@punitkoura
Copy link

I'm really new to the code base, though. Could you clarify what the cg_ssa means above?

@jonas-schievink
Copy link
Contributor

Could you clarify what the cg_ssa means above?

That's librustc_codegen_ssa, a part of the code generation code in rustc. The code in the first snippet is located there.

@punitkoura
Copy link

Thanks for clarifying that! I'll go through the code and post any questions here.

@TheOregonTrail
Copy link
Contributor

TheOregonTrail commented Aug 25, 2019

Would love to help but don't know where to start. If mentoring is still possible please ping me or send me in the right direction somehow 👍 . @bjorn3 @Centril

@TheOregonTrail
Copy link
Contributor

or maybe @jonas-schievink if your up to the task :)

@jonas-schievink
Copy link
Contributor

@luigisHat I think you just need to look at src/librustc_codegen_ssa/mir/rvalue.rs and remove the check for is_unit that was mentioned. You can check https://rust-lang.github.io/rustc-guide/ for an overview of the compiler and instruction on how to build it and run tests.

@TheOregonTrail
Copy link
Contributor

@jonas-schievink Thanks seems to be a little simpler then I thought once I dug into the docs. Will do hope your having a good one 👍 .

bors added a commit that referenced this issue Sep 18, 2019
Attempted fix to issue #63551

Well this is my first PR so hopefully I didn't do too bad.

As said I'm attempting to fix issue #63551 and remove the unnecessary check for is_unit.

would recommend removing `let is_unit = input_ty.is_unit()` unless there is another use.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 20, 2019
Remove unreachable unit tuple compare binop codegen

Closes rust-lang#63906
Fixes rust-lang#63551

This is based on rust-lang#63906 by @luigisHat, who had trouble with rebasing his PR.
@bors bors closed this as completed in 4f74fd7 Oct 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants