Wasm baseline: float.js test failure when run with --no-sse3

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: lth, Assigned: bbouvier)

Tracking

unspecified
mozilla56
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 disabled, firefox56 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
Created attachment 8877566 [details]
Reduced test case

The f32.floor test in wasm/float.js computes a NaN rather than 40 as it should when the baseline compiler is run with --no-sse3.  (Other tests in that file may fail too in this configuration, haven't checked yet.)  This is particularly odd, as the baseline compiler generates a callout for floor always; ABI problem?

Only a problem on x86, ie, 32-bit.  Looks like it is both optimized and non-optimized builds, both DEBUG and not.

Also happens on 32-bit Windows 7 runs.

Locally I'm compiling with -mfpmath=sse -msse -msse2, but on try (where I first saw this) I'm using whatever flags are the default, so I don't think that's it.

Beats me why we haven't seen this before, maybe --wasm-always-baseline is not used as extensively as we think it should be?  Tiering will take care of that...
(Reporter)

Comment 1

4 months ago
I guess this blocks tiering, then.
Blocks: 1277562
(Reporter)

Comment 2

4 months ago
Looking at these try runs, there are additional failures for --no-sse3.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1a3573aa167d777787c6de97b2acffd5a18b532&selectedJob=106943751
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1a3573aa167d777787c6de97b2acffd5a18b532&selectedJob=106954494

(I've repro'd the linux results locally on mozilla-inbound c74284491275, my massive patch queue that is underneath those runs is not needed.)
(Assignee)

Comment 3

4 months ago
[Tracking Requested - why for this release]: correctness issues in wasm for x86 machines *without* SSE4 support.

Bug 1340219 changed the ABI: return paths of floating-point functions don't need to do the x87 FPU dance anymore, under wasm ABI.
Assignee: lhansen → bbouvier
Blocks: 1340219
status-firefox55: --- → affected
status-firefox56: --- → affected
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
(Assignee)

Comment 4

4 months ago
owait, we don't need to track: the wasm baseline compiler isn't enabled there.
tracking-firefox55: ? → ---
tracking-firefox56: ? → ---
(Assignee)

Comment 5

4 months ago
Created attachment 8877611 [details] [diff] [review]
fix.patch
Attachment #8877611 - Flags: review?(lhansen)
(Reporter)

Comment 6

4 months ago
Comment on attachment 8877611 [details] [diff] [review]
fix.patch

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

Excellent, thank you.
Attachment #8877611 - Flags: review?(lhansen) → review+

Comment 7

4 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31758734ff3c
wasm baseline: don't read returned values from x87 FPU stack on x86; r=lth
https://hg.mozilla.org/mozilla-central/rev/31758734ff3c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Did you want to request beta uplift on this one, or is this wontfix for 55 because baseline is unused?
Flags: needinfo?(bbouvier)
(Assignee)

Comment 10

4 months ago
The baseline compiler isn't enabled by default in 55 (can be enabled through a pref though). I don't think there's any intent to uplift a patch that would enable the wasm baseline compiler. So we don't need to uplift this patch. Thanks for the heads-up!
Flags: needinfo?(bbouvier)
status-firefox54: --- → unaffected
status-firefox55: affected → disabled
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.