Closed Bug 1435146 Opened 2 years ago Closed 2 years ago
Vixl must handle restricted shift counts for Add / Sub with extended register
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.)
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
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?
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.
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
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/3bbc8c689dd3 Import VIXL PreShiftImmMode for MoveImmediate. r=lth
You need to log in before you can comment on or make changes to this bug.