trans: Call fmod manually for 32-bit float rem
#27875
Conversation
|
r? @pcwalton (rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @nrc This makes me a little sad, but I was unable to think of a cleaner way to do this :( |
|
I wonder if we can just remove this branch? Theoretically the trait implementation itself should work fine... right? (At least, it seems it wouldn't be backwards incompatible to rely on the trait, based on #27824 (comment)) |
|
Unfortunately we do actually hit this branch, even though there's a trait implementation in scope. For example this code: pub fn foo(a: f32, b: f32) -> f32 { a % b }generates this IR at O0 define float @_ZN3foo20hc71bdc578cf276aaeaaE(float, float) unnamed_addr #0 {
entry-block:
%dropflag_hint_6 = alloca i8
%dropflag_hint_9 = alloca i8
%a = alloca float
%b = alloca float
store i8 61, i8* %dropflag_hint_6
store i8 61, i8* %dropflag_hint_9
store float %0, float* %a, align 4
store float %1, float* %b, align 4
%2 = load float, float* %a, align 4
%3 = load float, float* %b, align 4
%4 = frem float %2, %3
ret float %4
} |
| // these two implementations on day! | ||
| let use_trait = tcx.sess.target.target.options.is_like_msvc && | ||
| tcx.sess.target.target.arch == "x86" && | ||
| lhs_t == tcx.types.f32; |
There was a problem hiding this comment.
You can't make this conditional on the operand type: you'll run into trouble in optimized builds if someone writes:
#![crate_type="lib"]
pub fn foo(a: f32, b: f32) -> f32 {
(a as f64 % b as f64) as f32
}There was a problem hiding this comment.
I believe lhs_t is the true type of the operation (e.g. with all types resolved), for example when I run this function through this patch on 32-bit Windows it generates a frem call with 64-bit doubles.
There was a problem hiding this comment.
This is with optimization turned on? (The problem is that instcombine narrows frem instructions.)
There was a problem hiding this comment.
Ah right of course, I'll have to poke around and see if I can't get something to work.
alexcrichton
force-pushed
the
msvc-f32-rem
branch
4 times, most recently
from
717ddbe
to
bccd32a
Compare
|
Well it looks like @eefriedman's right and LLVM is too smart for it's own good, I'm not sure how to subvert its logic to get it to not generate frem instructions, so closing for now. |
|
Well, you could change trans so it never generates |
Currently `f32 % f32` will generate a link error on 32-bit MSVC because LLVM will lower the operation to a call to the nonexistent function `fmodf`. Work around in this in the backend by lowering to a call to `fmod` instead with necessary extension/truncation between floats/doubles. Closes rust-lang#27859
|
That does indeed seem to work! (pushed) |
alexcrichton
force-pushed
the
msvc-f32-rem
branch
from
bccd32a
to
8a7b0fa
Compare
|
I've just made a patch for LLVM which causes |
|
Awesome, thanks @dylanmckay! If that lands I'll see about updating possibly updating LLVM or I'll add a note here saying that this branch should be removed once upstream LLVM has been updated. |
|
No problem @alexcrichton! With regards to updating LLVM - I am happy to do that. My AVR fork of Rust is easiest to update right after an LLVM upgrade :) |
|
@bors: r+ |
|
📌 Commit 8a7b0fa has been approved by |
|
⌛ Testing commit 8a7b0fa with merge c8c14f2... |
Currently `f32 % f32` will generate a link error on 32-bit MSVC because LLVM will lower the operation to a call to the nonexistent function `fmodf`. Work around in this in the backend by lowering to a call to `fmod` instead with necessary extension/truncation between floats/doubles. Closes #27859
The old code is temporarily needed in order to keep the MSVC build working. It should be possible to remove this code after the bootstrap compiler is updated to contain the MSVC workaround from rust-lang#27875.
|
This was fixed upstream in r246615. |
Currently
f32 % f32will generate a link error on 32-bit MSVC because LLVMwill lower the operation to a call to the nonexistent function
fmodf. Workaround in this in the backend by lowering to a call to
fmodinstead withnecessary extension/truncation between floats/doubles.
Closes #27859