1219050 - ARM64: Update to VIXL 1.10

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: 0001-Update-VIXL-helper-code-to-v1.10.patch

The attached patch updates our in-tree copy of VIXL from a hybrid of 1.2 and 1.3 to 1.10.

The patch passes all tests that don't time out regularly, except for sunspider/check-bitops-nsieve-bits, which returns a garbage object from UnboxedLayout::makeConstructorCode().

Version 1.10 adds support for vector operations necessary for SIMD.js, and includes exclusive access simulation necessary for SharedArrayBuffer.

I have attempted to carry over the changes made to VIXL in our tree during the update. Future updates should be easier to manage, since now that we have a set version, it will be easy to just update based on the diff.

Comment 1

[Sean Stangl [:sstangl]]

Mostly just as a note for myself: the problem with UnboxedLayout is likely that it doesn't save/restore LR on ARM/ARM64/MIPS, but calls allocateObject(). If the nursery happens to be full, we perform a call that clobbers LR.

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8679725 [details] [diff] [review]
0001-Update-VIXL-helper-code-to-v1.10.patch

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

Thanks, Sean

It's great to get this code updated!

I think there are problems with the order of include files in some of the .cpp files. Run "make check-style" to verify.

I don't think we should import the two new CompilerIntrinsics-vixl.* files since we already have all the same functions implemented in mfbt/MathAlgorithms.h, and they aren't used in that many places. How about including <mozilla/MathAlgorithms.h> from Utils-vixl.h, and calling the corresponding mozilla::* functions directly? Architecture-arm64.h already does this.

The changes to Debugger-vixl.cpp don't look correct and/or necessary to me. Most things in that file are allocated with js_new and should be freed with js_delete, and it doesn't seem necessary to switch range-based for loops back to indexed for loops. The additions like the new destructor look good.

MacroAssembler-vixl.cpp is losing a number of assertions about the reuse of scratch registers. Was this intentional?

Otherwise, looks good!

::: js/src/jit/arm64/vixl/Assembler-vixl.cpp
@@ +27,5 @@
>  
>  #include <cmath>
>  
> +#include "jit/arm64/vixl/Assembler-vixl.h"
> +#include "jit/arm64/vixl/MacroAssembler-vixl.h"

Should Assembler-vixl.h be included first, even before <cmath>?

https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#.23include_ordering

@@ +41,1 @@
>    VIXL_ASSERT((1 << index) & list_);

This looks a bit weird: list_ is a uint64_t, but (1 << index) is an int. (I know you didn't add it.)

If index >= 31, this assertion is undefined behavior, and probably doesn't work as expected.

I guess since list_ is not 0 at this point, and CPU register numbers are <= 31, this is not a problem.

@@ +53,1 @@
>    index = kRegListSizeInBits - 1 - index;

CountLeadingZeros() as a template is tricky because its result depends on the size of the argument type (is list_ a 32-bit or 64-bit type?). 

Utils-vixl.h just added the better HighestSetBitPosition():

int index = HighestSetBitPosition(list_)

::: js/src/jit/arm64/vixl/Assembler-vixl.h
@@ +837,4 @@
>  // Assembler.
>  class Assembler : public MozBaseAssembler {
>   public:
> +  Assembler(PositionIndependentCodeOption pic = PositionIndependentCode);

Should this be an explicit constructor?

@@ +878,4 @@
>      DoubleEqual                          = Condition::eq,
>      DoubleNotEqual                       = Condition::ne | DoubleConditionBitSpecial,
>      DoubleGreaterThan                    = Condition::gt,
> +    DoubleGreaterThanOrEqual             = Condition::ge, 

Spurious trailing whitespace.

::: js/src/jit/arm64/vixl/CompilerIntrinsics-vixl.h
@@ +25,5 @@
> +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +
> +#ifndef VIXL_COMPILER_INTRINSICS_H
> +#define VIXL_COMPILER_INTRINSICS_H

We already have our own versions of these functions in mfbt/MathAlgorithms.h, complete with compiler feature checks etc.

I wonder if it would be better for portability if we dropped CompilerIntrinsics-vixl.cpp and just provided inline stubs here forwarding to the mfbt functions?

For example, this header seems to fall back to the slow defaults on Windows.

::: js/src/jit/arm64/vixl/Debugger-vixl.cpp
@@ +252,4 @@
>   public:
>    explicit DebugCommand(Token* name) : name_(IdentifierToken::Cast(name)) {}
>    DebugCommand() : name_(NULL) {}
> +  virtual ~DebugCommand() { js_free(name_); }

name_ is an IdentifierToken* which I think was allocated with js_new<>().

@@ +304,4 @@
>   public:
>    StepCommand(Token* name, IntegerToken* count)
>        : DebugCommand(name), count_(count) {}
> +  virtual ~StepCommand() { js_free(count_); }

Same: Allocated with js_new<>().

@@ +1471,5 @@
>  
>  UnknownCommand::~UnknownCommand() {
> +  const size_t size = args_.length();
> +  for (size_t i = 0; i < size; ++i) {
> +    js_free(args_[i]);

What's going on here?

::: js/src/jit/arm64/vixl/Debugger-vixl.h
@@ +31,2 @@
>  #include <limits.h>
> +#include <errno.h>

You should run "make check-style" for some include order errors.

::: js/src/jit/arm64/vixl/Decoder-vixl.cpp
@@ +149,1 @@
>    visitors_.append(new_visitor);

This comment in ARM's original code is plain wrong:

  // We reached the end of the list. The last element must be
  // registered_visitor.
  VIXL_ASSERT(*it == registered_visitor);
  visitors_.insert(it, new_visitor);

Their |it| iterator would point at end() at this point which cannot legally be dereferenced.

::: js/src/jit/arm64/vixl/Instructions-vixl.cpp
@@ +27,2 @@
>  #include "jit/arm64/vixl/Assembler-vixl.h"
> +#include "jit/arm64/vixl/Instructions-vixl.h"

check-style

::: js/src/jit/arm64/vixl/MacroAssembler-vixl.cpp
@@ +280,5 @@
> +  } else {
> +    // TODO: Nede to register a slot in a literal pool, so that we can
> +    // write a branch instruction there and use that to branch in case
> +    // the unbound label winds up being out of range.
> +    tbz(rt, bit_pos, label);

s/Nede/Need/ (two cases)

I'm working on this as part of the fix for bug 1210554.

@@ -197,5 @@
>        Operand imm_operand = MoveImmediateForShiftedOp(temp, immediate);
>  
> -      // VIXL can acquire temp registers. Assert that the caller is aware.
> -      VIXL_ASSERT(!temp.Is(rd) && !temp.Is(rn));
> -      VIXL_ASSERT(!temp.Is(operand.maybeReg()));

Were these assertions removed intentionally?

::: js/src/jit/arm64/vixl/Simulator-vixl.cpp
@@ +1543,5 @@
> +    case REV_w: set_wreg(dst, ReverseBytes(wreg(src), 2)); break;
> +    case REV32_x: set_xreg(dst, ReverseBytes(xreg(src), 2)); break;
> +    case REV_x: set_xreg(dst, ReverseBytes(xreg(src), 3)); break;
> +    case CLZ_w: set_wreg(dst, CountLeadingZeros(wreg(src))); break;
> +    case CLZ_x: set_xreg(dst, CountLeadingZeros(xreg(src))); break;

Same as before: It's not obvious we're getting the right version of CountLeadingZeros<T>().

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79d2c5f174cc

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for arm64 SM failures: https://treeherder.mozilla.org/logviewer.html#?job_id=16706323&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/7688ea495057

Comment 5

[Sean Stangl [:sstangl]]

Clearing NI: crashes are addressed by a small patch to the (unused) "new" allocation, and by Bug 1222640.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e665ea64eb15

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e665ea64eb15