Add support for non-trapping float-to-int conversions. #1071

nlewycky · 2019-12-16T23:26:36Z

Description

Rewrites LLVM implementation of non-trapping float-to-int conversions. LLVM's conversion operations produce unspecified output when the input floats are out of range for the conversion.

Add implementation to singlepass for both x86-64 and AArch64.

Review

Add a short description of the the change to the CHANGELOG.md file

syrusakbary · 2019-12-16T23:54:38Z

bors try

bors · 2019-12-17T00:38:09Z

try

Build succeeded

continuous-integration/travis-ci/push
wasmerio.wasmer

MarkMcCaskey

Looks pretty good to me as far as code structure goes. The biggest code smell to me is the large number of repeated large magic numbers, sometimes with subtle differences. I didn't comment on all them, but I think it'd be good to give them names maybe put them somewhere shared if we're going to need them in every backend

MarkMcCaskey · 2019-12-18T16:58:06Z

lib/llvm-backend/src/code.rs

+    lower_bound: u64, // Exclusive (lowest representable value)
+    upper_bound: u64, // Exclusive (greatest representable value)
+    int_min_value: u64,
+    int_max_value: u64,


Comment in the abstract, I don't expect you to fix this in this PR or necessarily soon, but on first glance, I'd prefer a struct with methods over numbers in a case like this.

After reviewing the PR I think it's not as bad as I first thought. I think it's probably possible to clean it up a bit though. Low priority all things considered though

I'm not sure I understand the proposed cleanup here. Create a struct with lower/upper/min/max and a single method on it?

Just being more semantic where possible. It's a minor thing but a number is a very open ended thing. If it was a bunch of 0-sized structs with a method that returned an upper and lower bound, then mix ups would be less likely.

I don't think it's worth the effort here though -- I thought this was more complex than it actually is

MarkMcCaskey · 2019-12-18T17:01:51Z

lib/llvm-backend/src/code.rs

+                    std::i32::MIN as u64,
+                    2147483520u64, // bits as f32: 0x4effffff
+                    std::i32::MIN as u32 as u64,
+                    std::i32::MAX as u32 as u64,


⛳️ i32::max_value() doesn't need std:: 🤔 either way is fine though

I think std::i32::MIN is shorter than i32::max_value()? I tried i32::MIN and the compiler told me I need a std:: prefix.

Yeah, I'm fine with either -- the only reason I mention it is that I haven't seen the MIN and MAX constants in a while. Now that I see them again I do remember having seen them in the past.

MarkMcCaskey · 2019-12-18T17:06:21Z

lib/singlepass-backend/src/codegen_x64.rs

+                    &mut self.machine,
+                    tmp_in,
+                    -2147483904.0,
+                    2147483648.0,


Names and/or explanations for numbers like these are always appreciated

Added a section of constants to the bottom of the file. (Same in the LLVM backend too.) Done.

MarkMcCaskey · 2019-12-18T17:06:48Z

lib/singlepass-backend/src/codegen_x64.rs

+                    &mut self.machine,
+                    tmp_in,
+                    -9223373136366403584.0,
+                    9223372036854775808.0,


GEF32_LT_I64_MIN and LEF32_GT_I64_MAX.

MarkMcCaskey · 2019-12-18T17:07:15Z

lib/singlepass-backend/src/codegen_x64.rs

+                    &mut self.machine,
+                    tmp_in,
+                    -1.0,
+                    18446744073709551616.0,


here too, if these numbers are shared between the backends it might even make sense to put them in a shared location

These aren't quite shared between backends, though they absolutely could be. Note that these are f32/f64 while the llvm-backend uses u64, also llvm-backend uses values that are inside the valid range while singlepass uses values outside the valid range. I don't think there's any underlying need for these differences, but it might be that these are expressed in the way most convenient for lowering code in each backend. I'm going to punt that investigation and possible cleanup to a future PR.

MarkMcCaskey · 2019-12-18T17:08:21Z

lib/singlepass-backend/src/codegen_x64.rs

+                            a.emit_cvttss2si_64(XMMOrMemory::XMM(tmp_in), tmp_out);
+                            a.emit_mov(
+                                Size::S64,
+                                Location::Imm64(0x8000000000000000u64),


Could this be i64::min_value() as u64 or 1 << 63?

Yes, though I don't know what about this value to emphasize, I merely copied this code from I64TruncUF32. I'm going to request that I save that change for a future PR. This constant appears in numerous other places, including those not related to truncation at all.

MarkMcCaskey · 2019-12-18T17:09:22Z

lib/singlepass-backend/src/codegen_x64.rs

+                        &mut self.machine,
+                        tmp_in,
+                        -1.0,
+                        4294967296.0,


This number is appearing often

MarkMcCaskey · 2019-12-18T17:09:31Z

lib/singlepass-backend/src/codegen_x64.rs

+                    &mut self.machine,
+                    tmp_in,
+                    -1.0,
+                    4294967296.0,


perhaps a named constant is in order

MarkMcCaskey · 2019-12-18T17:09:45Z

lib/singlepass-backend/src/codegen_x64.rs

+                    &mut self.machine,
+                    real_in,
+                    -2147483649.0,
+                    2147483648.0,


MarkMcCaskey · 2019-12-18T17:11:01Z

lib/singlepass-backend/src/codegen_x64.rs

+                    &mut self.machine,
+                    tmp_in,
+                    -9223372036854777856.0,
+                    9223372036854775808.0,


The bottom number is the same but the top number is slightly different:

from

-9223373136366403584.0, 9223372036854775808.0,

names would make that more obvious

GEF64_LT_I64_MIN and LEF64_GT_I64_MAX.

losfair · 2019-12-20T17:11:26Z

lib/singlepass-backend/src/codegen_x64.rs

+                        if a.arch_has_itruncf() {
+                            a.arch_emit_i64_trunc_uf32(tmp_in, tmp_out);
+                        } else {
+                            let tmp = m.acquire_temp_gpr().unwrap(); // r15


Seems that these specific register indexes (r15, xmm1, xmm3) are not depended on from anywhere else?

Copied from I64TruncUF32. Agreed. I also never checked what registers are really chosen, so the comments could very well be wrong. Removed the comments.

losfair · 2019-12-20T17:12:52Z

lib/singlepass-backend/src/codegen_x64.rs

+                self.value_stack.push(ret);
+
+                let tmp_out = self.machine.acquire_temp_gpr().unwrap();
+                let tmp_in = self.machine.acquire_temp_xmm().unwrap(); // xmm2


Same as above

losfair · 2019-12-20T17:14:12Z

lib/singlepass-backend/src/codegen_x64.rs

+                        if a.arch_has_itruncf() {
+                            a.arch_emit_i64_trunc_uf64(tmp_in, tmp_out);
+                        } else {
+                            let tmp = m.acquire_temp_gpr().unwrap(); // r15


MarkMcCaskey

Looks reasonable from my side -- I didn't check that Heyang's issues were resolved, this approval is just from my side.

Though I do think sharing the constants should happen before this PR merged, unless there's a good reason why putting them in runtime-core or a new shared backend crate isn't a good idea

edit: for the record: the constants in singlepass and llvm are entirely different so it doesn't make sense to share them. I didn't review the PR closely enough 😄

MarkMcCaskey · 2019-12-20T22:49:53Z

lib/singlepass-backend/src/codegen_x64.rs

+/// Greatest Exact Float (64 bits) less-than u64::MIN when rounding towards zero.
+const GEF64_LT_U64_MIN: f64 = -1.0;
+/// Least Exact Float (64 bits) greater-than u64::MAX when rounding towards zero.
+const LEF64_GT_U64_MAX: f64 = 18446744073709551616.0;


I'd prefer if these weren't duplicated; if they don't seem to fit into runtime-core, we could add backend-common crate for this kind of thing. I'd like to encourage code reuse among the backends in general

…_sat.

…e llvm backend.

This breaks all of conversions.wast on singlepass. LLVM and Cranelift pass.

…t failures.

…Arch64.

… there's been a release.

… really matter.

nlewycky · 2019-12-21T00:23:16Z

bors r+

1071: Add support for non-trapping float-to-int conversions. r=nlewycky a=nlewycky # Description Rewrites LLVM implementation of non-trapping float-to-int conversions. LLVM's conversion operations produce unspecified output when the input floats are out of range for the conversion. Add implementation to singlepass for both x86-64 and AArch64. # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Nick Lewycky <nick@wasmer.io>

bors · 2019-12-21T01:06:17Z

Build succeeded

continuous-integration/travis-ci/push
wasmerio.wasmer

nlewycky requested review from losfair, MarkMcCaskey and syrusakbary as code owners December 16, 2019 23:26

bors bot added a commit that referenced this pull request Dec 16, 2019

Try #1071:

a9f8a55

MarkMcCaskey reviewed Dec 18, 2019

View reviewed changes

losfair reviewed Dec 20, 2019

View reviewed changes

nlewycky force-pushed the feature/trunc-sat branch from 258c4f3 to 4f1c53f Compare December 20, 2019 22:33

MarkMcCaskey approved these changes Dec 20, 2019

View reviewed changes

nlewycky added 12 commits December 20, 2019 16:20

Use type traits to ensure that the float and int types match in trunc…

5b35313

…_sat.

Correct implementation of non-trapping float to int conversions in th…

0cfe08f

…e llvm backend.

Add tests for non-trapping float to int conversions.

6fe2f43

This breaks all of conversions.wast on singlepass. LLVM and Cranelift pass.

Initial implementatio of trunc-sat instructions in singlepass. 27 tes…

442c40f

…t failures.

Finish implementation of trunc_sat in singlepass x86-64.

d52c193

Enable non-trapping float to int conversions by default.

32ed6f2

Add support for non-trapping float to int conversions in singlepass+A…

b7929e6

…Arch64.

Add changelog entry.

f8d792c

Update changelog again, to move entry to new unreleased section since…

56fd664

… there's been a release.

Name the magic constants in the LLVM backend.

f00283a

Name the magic constants in the singlepass backend.

e738a9f

Remove comments with register names that might not be right and don't…

bba0129

… really matter.

nlewycky force-pushed the feature/trunc-sat branch from 29d65c0 to bba0129 Compare December 21, 2019 00:23

bors bot merged commit bba0129 into master Dec 21, 2019

bors bot deleted the feature/trunc-sat branch December 21, 2019 01:06

Add support for non-trapping float-to-int conversions. #1071

Add support for non-trapping float-to-int conversions. #1071

Conversation

nlewycky commented Dec 16, 2019 • edited Loading

Description

Review

syrusakbary commented Dec 16, 2019

bors bot commented Dec 17, 2019

try

Build succeeded

MarkMcCaskey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkMcCaskey Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkMcCaskey left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlewycky commented Dec 21, 2019

bors bot commented Dec 21, 2019

Build succeeded

nlewycky commented Dec 16, 2019 •

edited

Loading

MarkMcCaskey Dec 18, 2019 •

edited

Loading

MarkMcCaskey left a comment •

edited

Loading