wasm: support i64 arithmetic operators

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

Posted patch PatchSplinter Review
This turned out to be pretty straight-forward on x64. I had to add some new instructions to the assembler for mul/div/mod though.

Division by zero etc should trap, but for now this patch just matches the i32 behavior (returns 0 or INT_MIN).

I also didn't add the LDivPowTwoI or LDivOrModConstantI optimizations; I can file a followup bug to add those.
Attachment #8723552 - Flags: review?(sunfish)
Comment on attachment 8723552 [details] [diff] [review]
Patch

Review of attachment 8723552 [details] [diff] [review]:
-----------------------------------------------------------------

Cool!

::: js/src/asmjs/Wasm.cpp
@@ -477,5 @@
>        case Expr::I64DivS:
>        case Expr::I64DivU:
>        case Expr::I64RemS:
>        case Expr::I64RemU:
> -        return f.fail("NYI: i64");

The decoder is being fuzzed, so the validation logic here needs to fail cleanly on operators that aren't fully implemented, so that we don't crash later in codegen. Currently i64 div and rem are only implemented on x64.

::: js/src/asmjs/WasmIonCompile.cpp
@@ +2194,5 @@
>  static bool
>  EmitDivOrMod(FunctionCompiler& f, ExprType type, bool isDiv, MDefinition** def)
>  {
> +    MOZ_ASSERT(type != ExprType::I32 && type != ExprType::I64,
> +               "int div or mod must precise signedness");

While you're here, could you change the string to say "indicate" instead of "precise" here?
Attachment #8723552 - Flags: review?(sunfish) → review+
(In reply to Dan Gohman [:sunfish] from comment #1)
> The decoder is being fuzzed, so the validation logic here needs to fail
> cleanly on operators that aren't fully implemented, so that we don't crash
> later in codegen. Currently i64 div and rem are only implemented on x64.

That's handled by the #ifndefs in DecodeValType and DecodeExprType I think.
https://hg.mozilla.org/mozilla-central/rev/02e9d9afda63
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
https://hg.mozilla.org/integration/mozilla-inbound/rev/3861ebac90e69ca083c577a4623800521a655402
Quick follow-up to bug 1251225 to fix a possibly overflowing left shift (found by Coverity, rs=h4writer on irc)
Why is 02e9d9afda63 in beta, but not its followup 3861ebac90e6?
Flags: needinfo?(wkocher)
So from the looks of things[1], 3861ebac90e6 merged to m-c in the top-most push in the link. The bottom-most push is what got merged to aurora as 47.



1. https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&tochange=886b5480b578&fromchange=68d3781deda0
Flags: needinfo?(wkocher)
I guess the followup should be uplifted to beta as well. It sounds weird that the two patches landed in different version.

Also, the information shown in the commit [1] says the followup is landed in 47.0a1 but actually it is not in 47, which is an issue as well.

[1] https://hg.mozilla.org/mozilla-central/rev/3861ebac90e6
It landed to inbound before 47 merged to aurora and before m-c became 48. By the time it merged from inbound to m-c, 47 was already on aurora and m-c had become 48. I guess the metadata only checked the version from the original landing to inbound and not what m-c looked like after the merge.
You need to log in before you can comment on or make changes to this bug.