Closed
Bug 1252432
Opened 8 years ago
Closed 8 years ago
wasm: support i64 conversion operators
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(4 files)
17.02 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
19.16 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
22.09 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
30.31 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
We need the unary {wrap, extend, trunc, convert, reinterpret} operators to convert between i64 and other types.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8725213 -
Flags: review?(luke)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8725215 -
Flags: review?(bbouvier)
Comment 3•8 years ago
|
||
Comment on attachment 8725213 [details] [diff] [review] Part 1 - i32.wrap Review of attachment 8725213 [details] [diff] [review]: ----------------------------------------------------------------- Excellent, thanks!
Attachment #8725213 -
Flags: review?(luke) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Especially the double/float32-to-unsigned conversion is a bit tricky here. This is what I came up with after looking at GCC and Clang codegen for this, and I copied the most interesting cases from the wast tests. Dan, does the codegen look correct?
Attachment #8725394 -
Flags: review?(sunfish)
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8725215 [details] [diff] [review] Part 2 - i64.extend_s/i64.extend_u Review of attachment 8725215 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmIonCompile.cpp @@ +2351,5 @@ > return true; > } > > static bool > +EmitExtendI32(FunctionCompiler& f, MDefinition** def, bool isUnsigned) Oh |def| should be the last parameter, I'll fix that.
Comment 6•8 years ago
|
||
Comment on attachment 8725215 [details] [diff] [review] Part 2 - i64.extend_s/i64.extend_u Review of attachment 8725215 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, thank you for the patch! ::: js/src/jit/MIR.cpp @@ +3742,5 @@ > +{ > + MDefinition* input = this->input(); > + if (input->isConstant()) { > + int32_t c = input->toConstant()->toInt32(); > + int64_t res = isUnsigned() ? int64_t(uint32_t(c)) : int64_t(c); uber nit: the isUnsigned can go inside the int64_t()
Attachment #8725215 -
Flags: review?(bbouvier) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8725394 [details] [diff] [review] Part 3 - i64.trunc_u/i64.trunc_s Review of attachment 8725394 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/x64/CodeGenerator-x64.cpp @@ +1170,5 @@ > + } else { > + if (inputType == MIRType_Double) { > + masm.vcvttsd2sq(input, output); > + masm.cmpq(Imm32(1), output); > + masm.j(Assembler::Overflow, &trap); The one tricky thing here (and for Float32 below) is that -0x8000000000000000 has two meanings; it's the error sentinel value, but it can also be a valid return value, because it's exactly representable as a double. If we see that value, we need to check the input to see if it was actually that value, and if so, return it as a valid result. Ideally this should be done as out-of-line code, though that's an optimization that we can always do later :).
Attachment #8725394 -
Flags: review?(sunfish) → review+
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Dan Gohman [:sunfish] from comment #7) > The one tricky thing here (and for Float32 below) is that > -0x8000000000000000 has two meanings; it's the error sentinel value, but it > can also be a valid return value, because it's exactly representable as a > double. If we see that value, we need to check the input to see if it was > actually that value, and if so, return it as a valid result. Ah good point. And here I was worried about the unsigned case :) Since we're truncating, we need to do two comparisons to check if the input is in the range around -0x8000000000000000 right? Do you mind if I land this as is, file a followup bug, and needinfo you? I don't trust my floating point skills, especially when it comes to handling these edge cases.
Flags: needinfo?(sunfish)
Assignee | ||
Comment 9•8 years ago
|
||
This implements signed and unsigned i64 to f32/f64 conversions on x64. The algorithm is what g++ 5.3 emits on OS X, even with -march=native. wasm has the same semantics right? Clang generates something that might be more efficient (vpunpckldq/vsubpd/vhaddpd, no branches), but that requires SSE3 so we can always add that later.
Attachment #8726685 -
Flags: review?(sunfish)
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa16519da919 https://hg.mozilla.org/integration/mozilla-inbound/rev/3231cc3fd19e https://hg.mozilla.org/integration/mozilla-inbound/rev/36a77fdd0533
Assignee | ||
Comment 11•8 years ago
|
||
I pushed the first 3 patches, to avoid bitrot, and filed bug 1253581 for the truncate thing.
Flags: needinfo?(sunfish)
Keywords: leave-open
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #6) > > + int64_t res = isUnsigned() ? int64_t(uint32_t(c)) : int64_t(c); > > uber nit: the isUnsigned can go inside the int64_t() If we do that, the arms of the ?: will have different types (uint32_t and int32_t), and the compiler will probably convert one of them to match the other.
Comment 13•8 years ago
|
||
Comment on attachment 8726685 [details] [diff] [review] Part 4 - {f32,f64}.convert_{u,s}/i64 Review of attachment 8726685 [details] [diff] [review]: ----------------------------------------------------------------- Sounds good.
Attachment #8726685 -
Flags: review?(sunfish) → review+
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fa16519da919 https://hg.mozilla.org/mozilla-central/rev/3231cc3fd19e https://hg.mozilla.org/mozilla-central/rev/36a77fdd0533
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c1c767f1d938
Comment 17•8 years ago
|
||
Reinterpret has been done in another bug. I don't think there's any other operator according to comment 0. Closing as fixed, feel free to reopen if I overlooked something.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 18•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•