Closed Bug 1160672 Opened 4 years ago Closed 4 years ago

ARM64: Import VIXL

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: sstangl, Assigned: sstangl)

References

Details

Attachments

(2 files)

To avoid further cacophony as in Bug 1088326, I'm breaking up the ARM64 landing into a bunch of separate bugs, each of which can be landed independently, with a final bug finally integrating them into the build system.

This bug imports VIXL. The Assembler and MacroAssembler are from 1.5, I believe, while the rest of the codebase is newly imported from 1.6. The majority of Mozilla changes to VIXL have been separated out into Moz* files, which will enable easier updating in the future.

The VIXL codebuffer is a lot saner than our current buffer implementation, which involves a tremendous amount of hidden instruction patching. It would be nice to move to the VIXL buffer at a future time, if we can find a sufficient excuse.

All the VIXL code (including the Mozilla) changes is kept under the normal VIXL license with copyright given to ARM.
Attachment #8600502 - Flags: review?(Jacob.Bramley)
The Mozilla changes mostly concern the IonAssemblerBufferWithConstantPools, so throwing to dougc.
Attachment #8600503 - Flags: review?(dtc-moz)
Comment on attachment 8600503 [details] [diff] [review]
Part 2/2 - Add Mozilla VIXL modifications

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

Not well scrutinized but it looks plausible and it would be good to see ARM64 support land. You might want to ask someone else to review it if it needs to meet a high standard to land.

::: js/src/jit/arm64/vixl/MozAssembler-vixl.cpp
@@ +60,5 @@
> +ptrdiff_t Assembler::LinkAndGetOffsetTo(BufferOffset branch, Label* label) {
> +  if (armbuffer_.oom())
> +    return js::jit::LabelBase::INVALID_OFFSET;
> +
> +  // The label is bound: all uses are already linked.

Might is be clearer to put such comments within the if statement true block?

@@ +125,5 @@
> +  Instruction* ins = getInstructionAt(branch);
> +  VIXL_ASSERT(ins->IsUncondBranchImm());
> +
> +  // Encode the relative offset.
> +  b(ins, LinkAndGetInstructionOffsetTo(branch, label));

With the linked list of uses stored in the branch and a 26 bit signed field, does this give a 128MB maximum code size?  What about uses in conditional branches, do these have a 19 bit signed field and support only a 1MB code size?

@@ +131,5 @@
> +}
> +
> +
> +BufferOffset Assembler::b(Label* label, Condition cond) {
> +  // Flush the instruction buffer before calculating relative offset.

Is this 'flush' description accurate? More uses below.

@@ +400,5 @@
> +  // The load currently contains the js::jit::PoolEntry's index,
> +  // as written by InsertIndexIntoTag().
> +  uint32_t index = load->ImmLLiteral();
> +
> +  // Each entry in the literal pool is uint32_t-sized.

It's in units of uint32_t-sized, but constants can use multiple entries.

@@ +423,5 @@
> +    // The size is in units of Instruction (4bytes), not byte.
> +    union {
> +      struct {
> +        uint32_t size : 15;
> +        bool isNatural : 1;

Does the 'isNatural' flag get used for ARM64?
Attachment #8600503 - Flags: review?(dtc-moz) → review+
(In reply to Douglas Crosher [:dougc] from comment #2)
> With the linked list of uses stored in the branch and a 26 bit signed field,
> does this give a 128MB maximum code size?  What about uses in conditional
> branches, do these have a 19 bit signed field and support only a 1MB code
> size?

Yeah. This uses the same scheme as ARM, although ARM gracefully fails if the encoded offset is out of range, and we (for the moment) panic. Branches can be patched to link to constant pools or an extended jump table, although Marty's plan was to phase out the extended jump table in favor of pools. In practice all the longer branches use registers.

> Is this 'flush' description accurate? More uses below.

It doesn't always cause a flush (literal pool emission), but the code is intended to allow for literal pool emission if necessary before instruction generation.

> Does the 'isNatural' flag get used for ARM64?

No, but we also didn't implement any instruction iterators that attempt to skip over pool guards. I'm not sure whether we'll need that yet or not.
Comment on attachment 8600502 [details] [diff] [review]
Part 1/2 - Import VIXL 1.5-1.6

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

I have several comments but I can't see anything _broken_ so I've given it r+. (I'm not sure where your acceptance threshold lies for these things.)

I generated diffs based on VIXL 1.5 (for Assmebler and MacroAssembler) and 1.6 (for everything else). My review comments are based on _that_. That is, I'm trying to review only what you've changed from the released VIXL.

Note that some parts appear to be from 1.7. For example, support for PRFM was added in 1.7, and it is present here too. Another example is the fixed disassembly for movz and movn, which was broken in 1.6. These things may have been cherry-picked, but it's hard to tell.

::: js/src/jit/arm64/vixl/Assembler-vixl.cpp
@@ +37,5 @@
> +
> +// CPURegList utilities.
> +CPURegister CPURegList::PopLowestIndex() {
> +  if (IsEmpty())
> +    return NoCPUReg;

In VIXL, we use curly brackets for 'if' statements that are split onto multiple lines (though this one could actually be a one-liner). There are many other style changes here (compared to VIXL). Is that intentional?

I won't identify other instances; they're just noise in the review.

@@ +378,5 @@
> +  Emit(at, BR | Rn(xn));
> +}
> +
> +
> +void Assembler::blr(const Register& xn) {

Some of these instructions have been moved, so they aren't in the order that we wrote them in VIXL. There's nothing functionally wrong with that -- the order is not particularly significant -- but it will make future merges more difficult than they need to be.

::: js/src/jit/arm64/vixl/Assembler-vixl.h
@@ +191,5 @@
> +  bool IsValidOrNone() const {
> +    return IsValid() || IsNone();
> +  }
> +};
> +static const CPURegister noReg;

There's a `NoReg` (and `NoCPUReg` and `NoFPReg`) somewhere already. Perhaps they should all be declared together.

@@ +270,5 @@
> +const FPRegister NoFPReg;
> +const CPURegister NoCPUReg;
> +
> +#define DEFINE_REGISTERS(N)  \
> +constexpr Register w##N(N, kWRegSize);  \

Why `constexpr` here, but `MOZ_CONSTEXPR` above?

@@ +563,5 @@
> +                      unsigned shift_amount = 0);
> +  explicit MemOperand(Register base,
> +                      const Operand& offset,
> +                      AddrMode addrmode = Offset);
> +  explicit MemOperand(ptrdiff_t PCoffset);

MemOperand(ptrdiff_t) is new in the Mozilla version, but there's no implementation of it anyway so it should probably be removed.

@@ +675,5 @@
> +  // called before executing or copying code from the buffer.
> +  void FinalizeCode();
> +
> +#define COPYENUM(v) static const Condition v = vixl::v
> +#define COPYENUM_(v) static const Condition v = vixl::v##_

Why is this necessary? The Assembler class is already inside the `vixl` namespace.

@@ +806,5 @@
> +  void cbz(const Register& rt, int imm19);
> +  static void cbz(Instruction* at, const Register& rt, int imm19);
> +
> +  // Compare and branch to label if not zero.
> +  void cbnz(const Register& rt, Label* label);

Some instructions (including `cbnz`) appear to have missing definitions in the Mozilla version of VIXL. They were originally in assembler-a64.cc. If you aren't using them, you won't notice, but it might cause you trouble later.

@@ +1832,5 @@
> +  static bool IsImmLSUnscaled(ptrdiff_t offset);
> +  static bool IsImmLSScaled(ptrdiff_t offset, LSDataSize size);
> +
> +  BufferOffset Logical(const Register& rd, const Register& rn,
> +                       const Operand& operand, LogicalOp op);

The implementation of `Logical` has been removed (from assembler-a64.cc), but loads of instructions depend on it. Has it been moved somewhere else? I can't see it in this diff.

@@ +1914,5 @@
> +  ptrdiff_t LinkAndGetPageOffsetTo(BufferOffset branch, Label* label);
> +
> +  // A common implementation for the LinkAndGet<Type>OffsetTo helpers.
> +  template <int element_size>
> +  ptrdiff_t LinkAndGetOffsetTo(BufferOffset branch, Label* label);

There isn't actually any definition of this function.

Actually, this seems to be quite common; I've seen several cases where an implementation has been replaced, but the declarations haven't been changed to match.

::: js/src/jit/arm64/vixl/Cpu-vixl.cpp
@@ +1,2 @@
> +// -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> +// vim: set ts=8 sts=4 et sw=4 tw=99:

Only `Cpu-vixl.cpp`, `Platform-vixl.h` and `Utils-vixl.cpp` have a modeline. I don't know whether Mozilla expect them to be here or not, but they should probably be used consistently.

::: js/src/jit/arm64/vixl/Debugger-vixl.cpp
@@ +1505,5 @@
> +}
> +
> +}  // namespace vixl
> +
> +#endif  // USE_SIMULATOR

USE_SIMULATOR has been renamed to JS_ARM64_SIMULATOR.

::: js/src/jit/arm64/vixl/MacroAssembler-vixl.cpp
@@ +557,5 @@
> +  unsigned n, imm_s, imm_r;
> +  int reg_size = dst.size();
> +
> +  if (imm == 0 && !dst.IsSP()) {
> +    // Zero is always held in the zero register.

I don't recognise this special-case for moving 0, so I _think_ it's a Mozilla addition. It looks like you prefer "mov xd, xzr" to "mov xd, #0". Out of curiosity, is there any particular reason for this?

@@ +776,5 @@
> +void MacroAssembler::PushStackPointer() {
> +  PrepareForPush(1, 8);
> +
> +  // Pushing the current stack pointer leads to implementation-defined
> +  // behavior, which may be surprising. In particular,

If "stack pointer" means "sp", then this isn't quite true: in A64, it is _impossible_ to push the architectural stack pointer because rt == 31 encodes the zero register.

However, the comment is correct in that "str xt, [xt, #offset]!" has undefined behaviour, but really this is just a special case of MacroAssembler::Push where the base register is in the list of source registers.

We did talk about implementing something like this in VIXL, but we decided that the expected behaviour might vary and thus produce surprising (and possibly hard-to-debug) results for some users.

@@ +901,5 @@
> +  VIXL_ASSERT(!src2.Is(GetStackPointer64()) && !src2.Is(sp));
> +  VIXL_ASSERT(!src3.Is(GetStackPointer64()) && !src3.Is(sp));
> +
> +  // The JS engine should never push 4 bytes, since we're trying to keep
> +  // 8-byte alignment at all times for sanity.

Why 8-byte alignment? You need 16-byte alignment for procedure calls, so you must already fix the alignment.

8-byte alignment might have a performance benefit in case you push 4 bytes, then repeatedly push 8-byte values. Is that the reasoning? (If so, "for sanity" should be expanded.)

@@ +1389,5 @@
> +}
> +
> +
> +void MacroAssembler::StackCheck(int index, int value) {
> +#if 0

This is a Mozilla addition, yet it is disabled. Do you intend to enable it later? If not, it might be best to remove it, to simplify merges.

@@ +1414,5 @@
> +
> +
> +
> +void MacroAssembler::StackCheckPushPop(int index, int value, int direction) {
> +#if 0

Ditto.

::: js/src/jit/arm64/vixl/MacroAssembler-vixl.h
@@ +979,5 @@
> +    int code = sp_.code();
> +    if (code == kSPRegInternalCode) {
> +      code = 31;
> +    }
> +    return js::jit::Register::FromCode(code);

Does js::jit::Register care about the actual code? If not, why not let it have the value 63 (kSPRegInternalCode)? It allows *sp and *zr to be distinguished, which saves an awful lot of grief. (We learnt this the hard way; VIXL originally set them both to 31, as they are encoded.)

@@ +1095,5 @@
> +// Use this scope when you need a one-to-one mapping between methods and
> +// instructions. This scope prevents the MacroAssembler from being called and
> +// literal pools from being emitted. It also asserts the number of instructions
> +// emitted is what you specified when creating the scope.
> +class InstructionAccurateScope {

In VIXL, InstructionAccurateScope is enforced by MacroAssembler::allow_macro_instructions_. However, this field has been removed in Mozilla's imported version, so InstructionAccurateScope is no longer enforced properly. This is unwise! (In particular, the comment is now no longer correct; Mozilla's InstructionAccurateScope does _not_ prevent the MacroAssembler from being called.)

::: js/src/jit/arm64/vixl/Simulator-vixl.h
@@ +57,5 @@
> +// hardware.
> +//
> +// TODO: Provide controls to prevent the macro assembler from emitting
> +// pseudo-instructions. This is important for ahead-of-time compilers, where the
> +// macro assembler is built with USE_SIMULATOR but the code will eventually be

USE_SIMULATOR has been renamed to JS_ARM64_SIMULATOR.
Attachment #8600502 - Flags: review?(Jacob.Bramley) → review+
Thank you for the review! The missing functions are present in the other patch attached to this bug -- everything that was extensively modified to fit our codebase was moved into Moz* files still under the VIXL license, to make future merging easier.

(In reply to Jacob Bramley [:jbramley] from comment #4)
> ::: js/src/jit/arm64/vixl/MacroAssembler-vixl.cpp
> @@ +557,5 @@
> > +  unsigned n, imm_s, imm_r;
> > +  int reg_size = dst.size();
> > +
> > +  if (imm == 0 && !dst.IsSP()) {
> > +    // Zero is always held in the zero register.
> 
> I don't recognise this special-case for moving 0, so I _think_ it's a
> Mozilla addition. It looks like you prefer "mov xd, xzr" to "mov xd, #0".
> Out of curiosity, is there any particular reason for this?

I think that's a change Marty made, and I don't know the reason. Does it matter?

> ::: js/src/jit/arm64/vixl/MacroAssembler-vixl.h
> @@ +979,5 @@
> > +    int code = sp_.code();
> > +    if (code == kSPRegInternalCode) {
> > +      code = 31;
> > +    }
> > +    return js::jit::Register::FromCode(code);
> 
> Does js::jit::Register care about the actual code? If not, why not let it
> have the value 63 (kSPRegInternalCode)? It allows *sp and *zr to be
> distinguished, which saves an awful lot of grief. (We learnt this the hard
> way; VIXL originally set them both to 31, as they are encoded.)

That's a great idea. I'll file a new bug for that, since the ARM64 codebase is broken up into a ton of disjoint patches at the moment.

I also added the ARM license to the Firefox about:licenses page.
> (In reply to Jacob Bramley [:jbramley] from comment #4)
> > [...]
> > I don't recognise this special-case for moving 0, so I _think_ it's a
> > Mozilla addition. It looks like you prefer "mov xd, xzr" to "mov xd, #0".
> > Out of curiosity, is there any particular reason for this?
> 
> I think that's a change Marty made, and I don't know the reason. Does it
> matter?

As far as I know, it makes no difference, but Marty might know something that I don't.
You need to log in before you can comment on or make changes to this bug.