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

trans: Always lower to frem #33508

Merged
merged 1 commit into from
May 14, 2016
Merged

Conversation

alexcrichton
Copy link
Member

Long ago LLVM unfortunately didn't handle the 32-bit MSVC case of frem where
it can't be lowered to fmodf because that symbol doesn't exist. That was since
fixed in http://reviews.llvm.org/D12099 (landed as r246615) and was released in
what appears to be LLVM 3.8. Now that we're using that branch of LLVM let's
remove our own hacks and help LLVM optimize a little better by giving it
knowledge about what we're doing.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@sanxiyn
Copy link
Member

sanxiyn commented May 9, 2016

Theoretically, we check for LLVM >=3.6 in configure, so this would break people using external LLVM. In practice, I don't think there are such people who also use 32-bit MSVC, so I guess it's okay.

@huonw
Copy link
Member

huonw commented May 9, 2016

Huh, I was under the impression we didn't do float % float in the compiler.

Long ago LLVM unfortunately didn't handle the 32-bit MSVC case of `frem` where
it can't be lowered to `fmodf` because that symbol doesn't exist. That was since
fixed in http://reviews.llvm.org/D12099 (landed as r246615) and was released in
what appears to be LLVM 3.8. Now that we're using that branch of LLVM let's
remove our own hacks and help LLVM optimize a little better by giving it
knowledge about what we're doing.
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 12, 2016

📌 Commit 96b2288 has been approved by nikomatsakis

eddyb added a commit to eddyb/rust that referenced this pull request May 13, 2016
…ikomatsakis

trans: Always lower to `frem`

Long ago LLVM unfortunately didn't handle the 32-bit MSVC case of `frem` where
it can't be lowered to `fmodf` because that symbol doesn't exist. That was since
fixed in http://reviews.llvm.org/D12099 (landed as r246615) and was released in
what appears to be LLVM 3.8. Now that we're using that branch of LLVM let's
remove our own hacks and help LLVM optimize a little better by giving it
knowledge about what we're doing.
@bors
Copy link
Contributor

bors commented May 13, 2016

⌛ Testing commit 96b2288 with merge 7f8fa4b...

@bors
Copy link
Contributor

bors commented May 13, 2016

💔 Test failed - auto-win-msvc-32-opt

@alexcrichton
Copy link
Member Author

@bors: retry

  • git

On Fri, May 13, 2016 at 6:26 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-msvc-32-opt
http://buildbot.rust-lang.org/builders/auto-win-msvc-32-opt/builds/3420


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#33508 (comment)

@bors
Copy link
Contributor

bors commented May 13, 2016

⌛ Testing commit 96b2288 with merge 2b79e05...

bors added a commit that referenced this pull request May 13, 2016
trans: Always lower to `frem`

Long ago LLVM unfortunately didn't handle the 32-bit MSVC case of `frem` where
it can't be lowered to `fmodf` because that symbol doesn't exist. That was since
fixed in http://reviews.llvm.org/D12099 (landed as r246615) and was released in
what appears to be LLVM 3.8. Now that we're using that branch of LLVM let's
remove our own hacks and help LLVM optimize a little better by giving it
knowledge about what we're doing.
@bors bors merged commit 96b2288 into rust-lang:master May 14, 2016
@alexcrichton alexcrichton deleted the always-lower-frem branch May 25, 2016 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants