Closed Bug 1396342 Opened 2 years ago Closed 2 years ago

Different behavior in wasm baseline/ion

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: azakai, Assigned: lth)

Details

Attachments

(2 files, 2 obsolete files)

376 bytes, application/x-javascript
Details
1.46 KB, patch
luke
: review+
Details | Diff | Splinter Review
Attached file t.wasm (obsolete) —
Running these commands leads to different output:

js --no-wasm-ion a.js t.wasm
js --no-wasm-baseline a.js t.wasm

calling: func_97
   exception: RuntimeError: index out of bounds
calling: func_97
   result: 4294967296

The second result (without baseline) is what v8 also emits, so perhaps the issue is in baseline.
Attached file a.js (obsolete) —
A little more manual reduction leads to

(module
 (export "func_97" (func $0))
 (func $0 (result i32)
  (i64.gt_u
   (i64.const 1)
   (i64.div_s
    (i64.const -1)
    (i64.const 4294967296)
   )
  )
 )
)

Baseline returns 0, ion returns 1.
Alon, x64 Linux I assume?
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
This is a bug in the optimized divide-by-power-of-two path in emitQuotientI64.  The problem seems to be that it uses Imm32(c-1) to adjust before shifting rather than Imm64(c-1), and that is wrong in this case.  emitRemainderI64() uses Imm64(c-1).
Attached file div.js
Selfcontained test case, it should print 0 and 0 but prints -1 and -1 with the baseline compiler.
Attachment #8903983 - Attachment is obsolete: true
Attachment #8903984 - Attachment is obsolete: true
Emit the proper 64-bit add for the divide-by-power-of-two optimization.  (Other cases look good.)  Includes test case.
Attachment #8904214 - Flags: review?(luke)
Attachment #8904214 - Flags: review?(luke) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60fd99ec9e71
Wasm baseline, fix division-by-64-bit constant bug. r=luke
Keywords: checkin-needed
(In reply to Lars T Hansen [:lth] from comment #3)
> Alon, x64 Linux I assume?

Yes, I saw this on x64 linux.
https://hg.mozilla.org/mozilla-central/rev/60fd99ec9e71
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.