ARM64: Use InvertCondition to expand the range of conditional branches

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sstangl, Assigned: sstangl)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8633693 [details] [diff] [review]
0001-Use-InvertCondition-to-expand-the-range-of-condition.patch

ARM64 branch-to-offset instructions take different widths: unconditional branches have 26 bits of signed offset, but conditional branches only have 19 bits. We run out of bits pretty easily on some longer tests -- Baseline binds a label at the beginning of the code, then outputs conditional branches to that throughout.

Instead of using a branch to the literal pool, this works around the problem by inverting the condition, which allows using an unconditional branch to encode the far-off location.

The place where literal pools need to come into play is marked with a TODO, but that will be a larger patch and is best left to when some test case requires it (fuzzing will definitely generate many).

Please keep in mind that the VIXL files use VIXL style. The functions introduced here are imported from the newest VIXL version.

Fixes auto-regress/bug690650.js with --ion-eager and a bunch of others.
Attachment #8633693 - Flags: review?(hv1989)
Comment on attachment 8633693 [details] [diff] [review]
0001-Use-InvertCondition-to-expand-the-range-of-condition.patch

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

Do we need to special-case Always and Never in B() ?
Currently we assert for those, but we are not guaranteed they aren't Always/Never.
In one change we actually use Always?!

::: js/src/jit/arm64/MacroAssembler-arm64.h
@@ +1562,3 @@
>      }
>      void j(Label* dest) {
> +        B(dest, Always);

If I understand correctly B(xxx, Always) will assert?
Attachment #8633693 - Flags: review?(hv1989)
(Assignee)

Comment 2

3 years ago
Created attachment 8642644 [details] [diff] [review]
0001-Use-InvertCondition-to-expand-the-range-of-condition.patch

It looks like that code came from someone copying the equivalent ARM code without thinking. The invalid j() function is never referenced, so I just removed it.
Attachment #8633693 - Attachment is obsolete: true
Attachment #8642644 - Flags: review?(hv1989)
(Assignee)

Updated

3 years ago
Assignee: nobody → sstangl
Attachment #8642644 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/d18e3a6d5149
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.