Closed Bug 1252432 Opened 4 years ago Closed 4 years ago

wasm: support i64 conversion operators

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(4 files)

We need the unary {wrap, extend, trunc, convert, reinterpret} operators to convert between i64 and other types.
Attachment #8725213 - Flags: review?(luke)
Attachment #8725215 - Flags: review?(bbouvier)
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+
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)
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 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 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+
(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)
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)
Blocks: 1253581
I pushed the first 3 patches, to avoid bitrot, and filed bug 1253581 for the truncate thing.
Flags: needinfo?(sunfish)
Keywords: leave-open
(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 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+
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: 4 years ago
Resolution: --- → FIXED
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.