Closed Bug 1810688 Opened 2 years ago Closed 2 years ago

Correctly implement MacroAssembler::branchTruncateDoubleToInt32 on ARM64

Categories

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

Product:
Core
Component:
JavaScript Engine: JIT
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
111 Branch
Tracking Flags:
Tracking Status
firefox111 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(19 files)

Bug 1810688 - Part 1: Use Fcmp with double constant. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 2: Add digit separators to some large constants. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 3: Correctly implement branchTruncateDoubleToInt32 on ARM64. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 4: Use Fjcvtzs for branchTruncateDoubleMaybeModUint32 when available. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 5: Add some comments to MacroAssembler::branchTruncateDouble. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 6: Use non-signaling Add when no overflows are possible. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 7: Remove unused MacroAssembler methods. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 8: Remove Nun-boxing specific MacroAssembler methods from 64-bit platforms. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 9: Disallow ImmPtr(0) in favour of ImmPtr(nullptr) or ImmWord(0). r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 10: Move ARM64 MacroAssembler::{floor,floorf,ceil,ceilf} into their single callers. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 11: Handle a trivial FIXME note about possibly static functions. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 12: Consistently use Uxtw to zero the upper 32 bits on ARM64. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 13: Add more comments to MacroAssembler::{floor,ceil}{Float32,Double}ToInt32. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 14: Merge NaN with negative zero check in floor{Float32,Double}ToInt32. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 15: Merge NaN with negative zero check in trunc{Float32,Double}ToInt32. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 16: Avoid unnecessary failures in trunc{Double,Float32}ToInt32 for ARM64. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 17: Directly fail for values in [-0.5,-0) in round{Double,Float32}ToInt32. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 18: Avoid unnecessary failures in round{Double,Float32}ToInt32 for ARM64. r=jandem!
(deleted), text/x-phabricator-request
Details
Bug 1810688 - Part 19: Directly fail for values in [-0.5,-0) in round{Double,Float32}ToInt32 for x86-shared. r=jandem!
(deleted), text/x-phabricator-request
Details

Correctly implement MacroAssembler::branchTruncateDoubleToInt32 on ARM64 and some additional (mostly ARM64 specific) clean-ups.

The zero constant 0.0 doesn't need to be loaded in a separate temp register,
but instead can be encoded directly in the Fcmp instruction.

This makes it a bit easier to read the constants, because missing digits can
be more easy detected.

Depends on D166993

The previous code didn't actually implement truncation, so any double with a
fractional part was triggering the failure path.

Depends on D166994

Fjcvtzs wasn't actually used for branchTruncateDoubleMaybeModUint32.

Depends on D166995

The Add, Cmp, and B instruction sequence tests for INT64_MIN and
INT64_MAX, but that's not easy to see right away.

Depends on D166996

If overflows aren't possible, we can use Add instead of Adds.

Depends on D166997

These methods aren't used anymore.

Depends on D166998

ToPayload and ToValue are neither used nor useful when nun-boxing isn't used.

Depends on D166999

Inline the methods into their only callers to simplify the code.

Depends on D167001

It looks like Loong64 needs a similar fix to part 3.

Flags: needinfo?(zhaojiazhong-hf)

(In reply to Jan de Mooij [:jandem] from comment #12)

It looks like Loong64 needs a similar fix to part 3.

Thanks for your information, I have submited loong64 fix patch to bug 1810728.

Flags: needinfo?(zhaojiazhong-hf)
Severity: -- → S3
Priority: -- → P3
Severity: S3 → N/A
Priority: P3 → P1

Drive-by fix:

  • Remove unnecessary scratch register use in ceilDoubleToInt32.

Depends on D167063

We can combine the NaN with the existing negative zero check, which avoids the
additional Fcmp instruction.

Depends on D167064

Convert the input to a 64-bit instead of 32-bit integer, similar to the floor
and ceil methods. This ensures we don't repeatedly fail for inputs like
Math.trunc(0x7fff_ffff + 0.5).

Depends on D167066

Instead of adding 0.5 for values in [-0.5, -0) and later always taking the
failure branch, because the result is always 0, directly take the failure
path when the input is in [-0.5, -0).

Depends on D167067

Changes similar to part 16 to avoid repeatedly failing for inputs like Math.round(0x7fff_ffff + 0.1).

Depends on D167068

Duplicate of this bug: 1519213
Pushed by andre.bargull@gmail.com: https://hg.mozilla.org/integration/autoland/rev/92a6cbe11ba1 Part 1: Use Fcmp with double constant. r=jandem https://hg.mozilla.org/integration/autoland/rev/5a51bab18af1 Part 2: Add digit separators to some large constants. r=jandem https://hg.mozilla.org/integration/autoland/rev/091bc4e096d4 Part 3: Correctly implement branchTruncateDoubleToInt32 on ARM64. r=jandem https://hg.mozilla.org/integration/autoland/rev/828041e46623 Part 4: Use Fjcvtzs for branchTruncateDoubleMaybeModUint32 when available. r=jandem https://hg.mozilla.org/integration/autoland/rev/c620790821e8 Part 5: Add some comments to MacroAssembler::branchTruncateDouble. r=jandem https://hg.mozilla.org/integration/autoland/rev/124475b37249 Part 6: Use non-signaling Add when no overflows are possible. r=jandem https://hg.mozilla.org/integration/autoland/rev/61316566a8c2 Part 7: Remove unused MacroAssembler methods. r=jandem https://hg.mozilla.org/integration/autoland/rev/e3ecd2e00465 Part 8: Remove Nun-boxing specific MacroAssembler methods from 64-bit platforms. r=jandem https://hg.mozilla.org/integration/autoland/rev/bf7c143ca420 Part 9: Disallow ImmPtr(0) in favour of ImmPtr(nullptr) or ImmWord(0). r=jandem https://hg.mozilla.org/integration/autoland/rev/e0b1f6b79c01 Part 10: Move ARM64 MacroAssembler::{floor,floorf,ceil,ceilf} into their single callers. r=jandem https://hg.mozilla.org/integration/autoland/rev/8854d2912c9c Part 11: Handle a trivial FIXME note about possibly static functions. r=jandem https://hg.mozilla.org/integration/autoland/rev/7b75cfc88295 Part 12: Consistently use Uxtw to zero the upper 32 bits on ARM64. r=jandem https://hg.mozilla.org/integration/autoland/rev/2cc9d573aa5e Part 13: Add more comments to MacroAssembler::{floor,ceil}{Float32,Double}ToInt32. r=jandem https://hg.mozilla.org/integration/autoland/rev/b1758f40e0b7 Part 14: Merge NaN with negative zero check in floor{Float32,Double}ToInt32. r=jandem https://hg.mozilla.org/integration/autoland/rev/ea06cba70d99 Part 15: Merge NaN with negative zero check in trunc{Float32,Double}ToInt32. r=jandem https://hg.mozilla.org/integration/autoland/rev/65989e5b9ae1 Part 16: Avoid unnecessary failures in trunc{Double,Float32}ToInt32 for ARM64. r=jandem https://hg.mozilla.org/integration/autoland/rev/634ea72f2f8f Part 17: Directly fail for values in [-0.5,-0) in round{Double,Float32}ToInt32. r=jandem https://hg.mozilla.org/integration/autoland/rev/2022d4e30361 Part 18: Avoid unnecessary failures in round{Double,Float32}ToInt32 for ARM64. r=jandem https://hg.mozilla.org/integration/autoland/rev/0bdaddbae382 Part 19: Directly fail for values in [-0.5,-0) in round{Double,Float32}ToInt32 for x86-shared. r=jandem
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: