Replace in-tree rustc_apfloat with the new version of the crate
#113843
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| // run-pass | ||
| // compile-flags: -O -Zmir-opt-level=3 -Cno-prepopulate-passes | ||
| // min-llvm-version: 16.0 (requires APFloat fixes in LLVM) | ||
|
There was a problem hiding this comment. It is still a mystery to me why with There was a problem hiding this comment. Yeah, I'm not sure either. Investigating more into LLVM at this point didn't seem worthwhile to me if it's been fixed in LLVM 16 but I could be convinced otherwise 🙂 Perhaps the issue is that LLVM 14's bitcode reader fails to roundtrip the float? There was a problem hiding this comment. LLVM constant-folding is not a pass, but rather happens when instructions are attempted to be created. There are passes, however, that do non-trivial simplifications, and propagate constants from e.g. loads of constant globals (though that might be in constant-folding, too? unsure), which re-triggers the constant-folding logic that first happened when the instructions were built, to account for their new inputs (you can almost think of it as "replace all uses of X with Y" simply rebuilding all instructions using X, and then the constructors of those instructions see Y now, which lets them try constant-folding again, and maybe get farther this time). In fewer words: attempting to add a runtime LLVM There was a problem hiding this comment. Small addendum: you may see something different when testing calls (of either LLVM intrinsics or |
||
|
|
||
| // Regression test for a broken MIR optimization (issue #113407). | ||
| pub fn main() { | ||
| let f = f64::from_bits(0x19873cc2) as f32; | ||
| assert_eq!(f.to_bits(), 0); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // run-pass | ||
| // compile-flags: -O -Zmir-opt-level=3 -Cno-prepopulate-passes | ||
|
|
||
| // Regression test for a broken MIR optimization (issue #102403). | ||
| pub fn f() -> f64 { | ||
| std::hint::black_box(-1.0) % std::hint::black_box(-1.0) | ||
| } | ||
|
|
||
| pub fn g() -> f64 { | ||
| -1.0 % -1.0 | ||
| } | ||
|
|
||
| pub fn main() { | ||
| assert_eq!(f().signum(), g().signum()); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // run-pass | ||
| // check-run-results | ||
| // regression test for issue #109567 | ||
|
|
||
| fn f() -> f64 { | ||
| std::hint::black_box(-1.0) % std::hint::black_box(-1.0) | ||
| } | ||
|
There was a problem hiding this comment. This seems to be basically the same as tests/ui/const_prop/apfloat-remainder-regression.rs? Is it worth having both? There was a problem hiding this comment. I figured since there are two different issues, it was probably worth having two tests, but I really don't feel strongly either way. |
||
|
|
||
| const G: f64 = -1.0 % -1.0; | ||
|
|
||
| pub fn main() { | ||
| assert_eq!(-1, G.signum() as i32); | ||
| assert_eq!((-0.0_f64).to_bits(), G.to_bits()); | ||
| assert_eq!(f().signum(), G.signum()); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to figure out why this was originally here so that I can justify why its being removed now.
I got to this commit: 0743ed6
and now I'm stuck. Why were we clamping ldexp exponent to i16, and why don't we need to keep doing so now? I'm assuming its a weakness in the old APFloat API that has since been addressed in the new rustc_apfloat, but can you confirm that @wesleywiser ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI this is from Miri PR rust-lang/miri#902
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. The old APFloat API took ldexp as an
i16but the new crate takesi32which matches what the code originally did before calling into APFloat. There's additional discussion whyi16is sufficient for this parameter in the discussion here rust-lang/miri#902 (comment).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So in theory this could have stayed as-is, but since it was an artifact of a prior limitation, we're removing it. RIght?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I could have just added an
.into()to do thei16 -> i32conversion but since we're actually doing ai32 -> i16 -> i32conversion, it seemed cleaner to just remove the conversions entirely.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thanks for the nice cleanup. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can explain this one, it's llvm/llvm-project@8606d01
[APFloat] Enlarge ExponentType to 32bit integerfrom late 2019 - previously, miri was stuck with ani32andrustc_apfloatwantedi16, but they now both agree oni32.