Closed Bug 1435146 Opened 2 years ago Closed 2 years ago

Vixl must handle restricted shift counts for Add / Sub with extended register

Categories

(Core :: JavaScript Engine: JIT, enhancement)

ARM64
All
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: lth, Assigned: sstangl)

References

Details

Attachments

(1 file, 1 obsolete file)

For rd = rn +/- (rm SHIFT k), aarch64 requires k <= 4 (three-bit shift count field, with some unencoded values).  Vixl ignored this requirement and would generate unencoded instructions for large constants with enough zeroes at either end.
A very local fix.  (MoveImmediateForShiftedOp actually has UB when shift counts are large or when imm is negative, but I opted against trying to fix that in this patch, to keep it clean.)
Attachment #8947709 - Flags: review?(sstangl)
Hardware: Other → ARM64
Summary: ARM64: Vixl must handle restricted shift counts for Add / Sub with extended register → Vixl must handle restricted shift counts for Add / Sub with extended register
VIXL upstream grew the ability to handle these things, implemented differently. Note that VIXL is actively being developed by Linaro now.

We haven't pulled from them in a long time -- might it be time to finally do so, if we're working on AArch64 for real?

Their version is here: https://git.linaro.org/arm/vixl.git/tree/src/aarch64/macro-assembler-aarch64.cc#n1568
Flags: needinfo?(lhansen)
I see.  I see I also missed that only left shifts are allowed in this case :)

We /probably/ should pull from upstream but I would sure prefer not to block on it right now, given everything I'm doing.  Might we land this patch (after I fix it to deal with the illegal right shift) and then create a work item to pull in the new one?
Flags: needinfo?(lhansen)
Will not land this, will wait for a new vixl import instead (bug 1435968).
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
Importing all of VIXL might take a while, but if you need just these particular changes, it would be easy enough to import just that piece.
Imports just the VIXL changes to MoveImmediateForShiftedOp().

Since the full import will be a larger project, and I don't think you should be blocked on that, I just imported the changes to this one function (and to the other functions that referred to it).

Linaro has not changed the license terms under which VIXL is shipped, so there is no need to update the about:licenses.
Attachment #8947709 - Attachment is obsolete: true
Attachment #8947709 - Flags: review?(sstangl)
Attachment #8948838 - Flags: review?(lhansen)
Comment on attachment 8948838 [details] [diff] [review]
0001-Bug-1435146-Import-VIXL-PreShiftImmMode-for-MoveImme.patch

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

Thank you for doing this!
Attachment #8948838 - Flags: review?(lhansen) → review+
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee: lhansen → sstangl
Status: REOPENED → ASSIGNED
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bbc8c689dd3
Import VIXL PreShiftImmMode for MoveImmediate. r=lth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3bbc8c689dd3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.