Closed Bug 1420894 Opened 2 years ago Closed 2 years ago

Wasm: Redundant NaN path in wasmTruncateToInt32

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

ARM
All
enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: lth, Assigned: bobslept, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

On ARM, the float-to-int conversion instructions work as follows:

- nan is folded to zero
- integer underflows are folded to zero (unsigned) or INT32_MIN (signed)
- integer overflows are folded to UINT32_MAX (unsigned) or INT32_MAX (signed)

In wasmTruncateToInt32 there is first a compare of the input with itself followed by an OOL branch if the compare is unordered.

That check is redundant for the unsigned case that follows, because that case handles a zero result with an OOL branch anyway (for underflow); at most it should be used for the signed case.  So at least we should move the early check below the unsigned case.

The signed case could however perform the same check in fewer instructions by omitting the guard and instead comparing the result with zero; it is already comparing the result to the two extreme values.  (With predication this is simply a CMP.)  It is hard to know whether this is better or worse than the guard, because zero is probably a fairly likely result of the conversion and as it is we avoid going OOL on a zero result because of the NaN guard.
Mentor: lhansen
Keywords: good-first-bug
Option 1 (just move some code) is easy.  I recommend doing this.

Option 2 (fold the NaN check into the guards on the converted result) requires some benchmarking.

Option 3 is to actually do FP compares before the conversion since we can branch on condition-or-unordered; this way we can always guard in two compares.  But there's a cost, because it means we have to load FP literals to compare against, and that takes time and space (both data and instruction counts).
Could you please let me know where exactly I should be looking into?
Search for wasmTruncateToInt32 in js/src/jit/arm/MacroAssembler-arm.cpp.
Whiteboard: [lang=c++]
Attached patch 1420894-move-nan-check.patch (obsolete) — Splinter Review
I hope this patch is correct. I have placed the NaN check below the unsigned check.
Comment on attachment 8975287 [details] [diff] [review]
1420894-move-nan-check.patch

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

Lars can you take a look?
Attachment #8975287 - Flags: review?(lhansen)
Assignee: nobody → bobslept
Sure, I'll take a look this week.
Comment on attachment 8975287 [details] [diff] [review]
1420894-move-nan-check.patch

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

Good.  It would also be good if you could summarize in a brief comment above the unsigned case why the nan check is not needed there; something simple like "A NaN check is not necessary because NaN is converted to zero and on a zero result we branch out of line to do further processing anyway."
Attachment #8975287 - Flags: review?(lhansen) → review+
One more thing.  The commit comment should be a little more explanatory and speak more to intent than to mechanism; eg, "Optimize a check in float-to-int conversion".
Thanks for the information. I have added the comments and changed the commit message. I have also added r=lth to it.
Attachment #8975287 - Attachment is obsolete: true
Attachment #8976157 - Flags: review?(lhansen)
Comment on attachment 8976157 [details] [diff] [review]
1420894-move-nan-check.patch

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

Yeah, ship it :)
Attachment #8976157 - Flags: review?(lhansen) → review+
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b384ccfe2e50
Optimize a check in float-to-int conversion. r=lth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b384ccfe2e50
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.