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.

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

Stop forcing i1 booleans to i8 for comparison when LLVM is fixed #40980

Closed
bstrie opened this issue Apr 1, 2017 · 7 comments
Closed

Stop forcing i1 booleans to i8 for comparison when LLVM is fixed #40980

bstrie opened this issue Apr 1, 2017 · 7 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bstrie
Copy link
Contributor

bstrie commented Apr 1, 2017

The current behavior introduced by #36958 is a workaround for the bug at #36856 . In the comment at #36958 (comment) Niko mentions that this is worth removing when the upstream LLVM bug is fixed (per that comment's suggestion I'm also presumptuously tagging this as P-medium). I know we have an upgrade to LLVM 4.0 in the works ( #37609 ), so that may fix it. However, what is our policy on compiling on old versions of LLVM, for the sake of rustc distribution in distros that ship their own LLVM? If we need to remain compatible with old versions of LLVM, then the workaround may need to exist for quite a long time.
@bstrie bstrie added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 1, 2017
@nagisa
Copy link
Member

nagisa commented Apr 1, 2017

However, what is our policy on compiling on old versions of LLVM, for the sake of rustc distribution in distros that ship their own LLVM?

We still support 3.7.

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 27, 2017
@bstrie
Copy link
Contributor Author

bstrie commented Oct 20, 2017

The LLVM bug in question appears to be https://bugs.llvm.org/show_bug.cgi?id=30579 , which is now marked resolved. Presumably the workaround in #36958 can now be removed, so I'm marking this E-easy. However, don't just revert the commit itself; leave its test case intact.

@bstrie bstrie added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Oct 20, 2017
@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Oct 20, 2017 •
edited
Loading

Comment 2 on the LLVM bug indicates the fix is in LLVM 4.0, but we still support LLVM 3.9, so we can't actually do this just yet.

@krk
Copy link
Contributor

krk commented Apr 18, 2018

Is LLVM 3.9 still supported? @rkruppe

@hanna-kruppe
Copy link
Contributor

I'm not aware of any changes. Indeed we still have a CI image that builds with LLVM 3.9.

@jonas-schievink jonas-schievink mentioned this issue Apr 21, 2018
Merged
@hanna-kruppe
Copy link
Contributor

Huh, I've only just seen https://bugs.llvm.org/show_bug.cgi?id=30579#c3 -- if this is correct, there was never any need to work around it, we could have just dropped our (incorrect) FastISel patch. Does someone have to have a (non-rustc) LLVM 3.9 build available to try and reproduce?

@hanna-kruppe
Copy link
Contributor

@nox was so kind as to check, and indeed stock LLVM 3.9 doesn't have the bug.

bors added a commit that referenced this issue Apr 27, 2018
@bors
Auto merge of #50137 - nox:rm-bool-cmp-hack, r=eddyb
a997525 
Remove hack around comparisons of i1 values (fixes #40980)

The regression test still passes without that 2 years old hack. The underlying
LLVM bug has probably been fixed upstream since then.
@bors bors closed this as completed in 5d5fb97 Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nagisa @bstrie @krk @hanna-kruppe @Mark-Simulacrum