1245112 - Move MacroAssembler branch functions to the generic macro assembler.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Tooru Fujisawa [:arai]]

just like bug 1203964 and bug 1229057, Move declarations of branch related functions to the generic MacroAssembler.

branch32

branchPtr
branchPtrWithPatch
branchPtrInNurseryRange
branchPrivatePtr

branchFloat
branchTruncateFloat32

branchDouble
branchTruncateDouble

branchAdd32
branchSub32
decBranchPtr

branchNegativeZero
branchNegativeZeroFloat32

branchTest16
branchTest32
branchTest64

branchTestPtr

branchTestInt32
branchTestInt32Truthy

branchTestDouble
branchTestDoubleTruthy

branchTestNumber

branchTestBoolean
branchTestBooleanTruthy

branchTestString
branchTestStringTruthy

branchTestSymbol

branchTestUndefined

branchTestNull

branchTestPrimitive

branchTestMagic
branchTestMagicValue

branchTestGCThing
branchTestObject
branchTestValue
branchValueIsNurseryObject

Comment 1

[Tooru Fujisawa [:arai]]

will post some basic parts after test.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Move MacroAssembler::branchPrivatePtr into generic macro assembler.

as a first step, prepared 10 patches for some basic methods and depending things.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14a681aefa1f

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Move MacroAssembler::branchPtr into generic macro assembler.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Move MacroAssembler::branch32 into generic macro assembler.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8716641 [details] [diff] [review]
Part 1: Move MacroAssembler::branchPrivatePtr into generic macro assembler.

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

Thanks for looking at this technical debt. :)

As experienced, differences between platforms can highlight bugs in any platform.  Thus we should take this opportunity to question the differences and to comment them.

::: js/src/jit/MacroAssembler.h
@@ +807,5 @@
>  
> +    // ===============================================================
> +    // Branch functions
> +
> +    inline void branchPrivatePtr(Condition cond, Register lhs, ImmWord rhs, Label* label)

Remove this unused function.

@@ +811,5 @@
> +    inline void branchPrivatePtr(Condition cond, Register lhs, ImmWord rhs, Label* label)
> +        DEFINED_ON(arm, arm64, mips_shared);
> +    inline void branchPrivatePtr(Condition cond, const Address& lhs, Register rhs, Label* label)
> +        DEFINED_ON(arm, arm64, mips_shared, x86, x64);
> +    inline void branchPrivatePtr(Condition cond, const Address& lhs, ImmPtr rhs, Label* label)

Remove this unused function.

::: js/src/jit/x64/MacroAssembler-x64-inl.h
@@ +227,5 @@
> +{
> +    ScratchRegisterScope scratch(*this);
> +    if (rhs != scratch)
> +        movePtr(rhs, scratch);
> +    rshiftPtr(Imm32(1), scratch);

This code got added in Bug 856627 without any comment on why this is different from other architecture, such as x86 and arm.

As we added new backends since, such as mips32, mips64 and arm64, I definitely want to know if this is an optimization based on the word size or not.

I will cancel this review until we can figure this out, and add a proper comment and/or fix the code accordingly.

From what I can read, I cannot see any reason to shift this value by 1, the pointers does not seems to be boxed in any way.  This function is called from within the SharedIC variant of CheckDOMProxyExpandoDoesNotShadow, while the IonCache variant of it does not do that.

Looking at how the baseline IC is initialized let me think that this shift has no reason to exists, and thus that we can replace the branchPrivatePtr by a simple branchPtr.

=> ni? efaust, as this is related to DOM proxy code.

Comment 6

[Nicolas B. Pierron [:nbp]]

Eric, can you look at the Part 1, and tell us if this right shift is something that we should keep?

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8716642 [details] [diff] [review]
Part 2: Move MacroAssembler::branchPtr into generic macro assembler.

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

::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +428,5 @@
> +void
> +MacroAssembler::branchPtr(Condition cond, Register lhs, wasm::SymbolicAddress rhs, Label* label)
> +{
> +    ScratchRegisterScope scratch(*this);
> +    movePtr(rhs, scratch);

This sounds like an existing bug in the arm & mips-shared backend implementations, as

  branchPtr(Condition, Register, wasm::SymbolicAddress, Label*)
      uses movePtr

and

  branchPtr(Condition, wasm::SymbolicAddress, Register, Label*)
      uses loadPtr

=> ni? bbouvier, luke

@@ +448,5 @@
> +void
> +MacroAssembler::branchPtr(Condition cond, const Address& lhs, ImmGCPtr rhs, Label* label)
> +{
> +    AutoRegisterScope scratch2(*this, secondScratchReg_);
> +    ma_ldr(lhs, scratch2);

nit: use loadPtr instead of ma_ldr, and do the same for the functions which is below.

@@ +450,5 @@
> +{
> +    AutoRegisterScope scratch2(*this, secondScratchReg_);
> +    ma_ldr(lhs, scratch2);
> +    ma_cmp(scratch2, rhs);
> +    ma_b(label, cond);

nit: replace these 2 instructions by a call to
    branchPtr(cond, scratch2, rhs, label);

And do the same for the branchPtr functions which are below.

::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +508,5 @@
> +
> +    movePtr(rhs, scratch1_64.asUnsized());
> +    loadPtr(lhs, scratch2_64.asUnsized());
> +    cmp(scratch2_64, scratch1_64);
> +    B(cond, label);

existing-nit: use branchPtr here, instead of cmp & B functions.

::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +227,5 @@
> +void
> +MacroAssembler::branchPtr(Condition cond, Register lhs, wasm::SymbolicAddress rhs, Label* label)
> +{
> +    movePtr(rhs, SecondScratchReg);
> +    ma_b(lhs, SecondScratchReg, label, cond);

existing-nit: As other platform implementation, I will suggest to use branchPtr is there is already a movePtr / loadPtr in the body of the function.

::: js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h
@@ +244,5 @@
> +template <typename T, typename S>
> +void
> +MacroAssemblerX86Shared::branchPtrImpl(Condition cond, const T& lhs, const S& rhs, Label* label)
> +{
> +    asMasm().cmpPtr(Operand(lhs), rhs);

self-note: Operand variant of cmpPtr belongs to MacroAssembler{X86,X64}, and thus the asMasm() trick is just a way to reuse the inheritance.  Operand variants do not belong to the generic MacroAssembler because Operands are only specified per-architecture.

nit: Maybe we should copy this branchPtrImpl in both MacroAssemblerX86 and MacroAssemblerX64?  Or Maybe we could define the missing cmpPtr variants on the generic MacroAssembler, and not use the Operand(…) constructor.

::: js/src/jit/x86/MacroAssembler-x86-inl.h
@@ +272,5 @@
> +
> +void
> +MacroAssembler::branchPtr(Condition cond, wasm::SymbolicAddress lhs, Register rhs, Label* label)
> +{
> +    cmpl(rhs, lhs);

existing-nit: Note that operands are swapped!  This implies that either the condition flag should be swapped to or an assertion which ensure that this function is only called with
    cond == Equal || cond == NotEqual.

ni? bbouvier, luke

Comment 8

[Nicolas B. Pierron [:nbp]]

Benjamin and Luke, can you look at part 2 comments, and give some feedback about the branchPtr implementation with wasm::SymbolicAddress, both on arm & mips-shared, and also on x86-inl and x64-inl.

Comment 9

[Luke Wagner [:luke]]

I could be reading this wrong but it appears that, in Assembler-x86.h, all the cmpl overloads have lhs and rhs in the wrong order and the SymbolicAddress overloads is just copying that.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Luke Wagner [:luke] from comment #9)
> I could be reading this wrong but it appears that, in Assembler-x86.h, all
> the cmpl overloads have lhs and rhs in the wrong order and the
> SymbolicAddress overloads is just copying that.

Oh, thanks, I did not notice that this was cmpl and not cmpPtr, as all others.
I guess this is one more reasons for in favor of the generic MacroAssembler.

Comment 11

[Tooru Fujisawa [:arai]]

what about loadPtr vs movePtr in arm & mips-shared "Register lhs, wasm::SymbolicAddress rhs" cases in comment #7 ?

Comment 12

[Luke Wagner [:luke]]

Oops, sorry.  It looks like the movePtr variants are dead code from back when we had two types (to distinguish immediates (that you'd move) and addresses (that you'd load).  Now there is just SymbolicAddress and it means "load".

Comment 13

[Eric Faust [:efaust]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> From what I can read, I cannot see any reason to shift this value by 1, the
> pointers does not seems to be boxed in any way.  This function is called
> from within the SharedIC variant of CheckDOMProxyExpandoDoesNotShadow, while
> the IonCache variant of it does not do that.

> => ni? efaust, as this is related to DOM proxy code.

So, what this function does is takes an Address to a PrivateValue, and a void* and compares them. Rather than loadValue and unboxPrivate and branchPtr, it instead "boxes" the void* in a scratch register, and does a direct comparison with the PrivateValue. Since this process, on 64bits is a rshift, that's what we do. I admit that it's open-coded, but this seems like a fairly straightforward implementation of the function. I don't know what "keep" means in this context. Unifying it against others seems hard, since this is a 64-bit specific handling, but the rshift is totally mandatory for the correctness of branchPrivatePtr there.

Comment 14

[Eric Faust [:efaust]]

(In reply to Eric Faust [:efaust] from comment #13)
> I don't know what "keep" means in this context.

OK, Nicolas has explained more context to me. The difference in the callers is warranted: in baseline (SharedStubs), the PrivateValue is stored in the stub, so it's read by the jitcode, and both sides of the check are "dynamic". In Ion, we bake the private pointer directly into the stub's jitcode, so it's "statically" known. That's why the code snippets are different.

We can /still/ maybe remove this function, by just storing a void* in the stub, rather than a PrivateValue that we need to unbox. Maybe it's worth looking in to this.

Either way, Nicolas points out that we should have such a shift for all PUNBOX_64 architectures, which probably means fixing ARM64 and maybe MIPS64.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Move MacroAssembler::branchPrivatePtr into generic macro assembler.

Removed unused variants, splitted mips-shared into 32/64, added comment for x64, and applied same fix to arm64/mips64.

Comment 16

[Tooru Fujisawa [:arai]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> (In reply to Luke Wagner [:luke] from comment #9)
> > I could be reading this wrong but it appears that, in Assembler-x86.h, all
> > the cmpl overloads have lhs and rhs in the wrong order and the
> > SymbolicAddress overloads is just copying that.
> 
> Oh, thanks, I did not notice that this was cmpl and not cmpPtr, as all
> others.
> I guess this is one more reasons for in favor of the generic MacroAssembler.

so, should we swap the arguments of cmpl definition?

(actually I'm not sure which part of assembler/macro assembler follows which syntax or operand order...)

Comment 17

[Nicolas B. Pierron [:nbp]]

(In reply to Tooru Fujisawa [:arai] from comment #16)
> (In reply to Nicolas B. Pierron [:nbp] from comment #10)
> > (In reply to Luke Wagner [:luke] from comment #9)
> > > I could be reading this wrong but it appears that, in Assembler-x86.h, all
> > > the cmpl overloads have lhs and rhs in the wrong order and the
> > > SymbolicAddress overloads is just copying that.
> > 
> > Oh, thanks, I did not notice that this was cmpl and not cmpPtr, as all
> > others.
> > I guess this is one more reasons for in favor of the generic MacroAssembler.
> 
> so, should we swap the arguments of cmpl definition?

No cmpl is effectively taking its arguments in reversed order as opposed to cmpPtr. It attempts to follow the way the assembler syntax is written [1].

The only case where the arguments seems to be accidentally switched does not matter, as we are checking for NotEqual condition. [2]

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/x86-shared/BaseAssembler-x86-shared.h#1400
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp#1099

Comment 18

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8717772 [details] [diff] [review]
Part 1: Move MacroAssembler::branchPrivatePtr into generic macro assembler.

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

Thanks :)

::: js/src/jit/MacroAssembler.h
@@ +800,5 @@
>  
> +    // ===============================================================
> +    // Branch functions
> +
> +    inline void branchPrivatePtr(Condition cond, const Address& lhs, Register rhs, Label* label) PER_ARCH;

I think it is important to comment above this function, as both inputs have different types:

  // This function compares a Value (lhs) which is having a private pointer boxed
  // inside a js::Value, with a raw pointer (rhs).

Comment 19

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8716643 [details] [diff] [review]
Part 3: Move MacroAssembler::branch32 into generic macro assembler.

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

Apart from existing issues in the arm variant of the branch32 comments, which deserves its own bug. I do not see any issue with this patch.

::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +482,5 @@
> +{
> +    ScratchRegisterScope scratch(*this);
> +    loadPtr(lhs, scratch);
> +    ma_cmp(scratch, rhs);
> +    ma_b(label, cond);

nit: use branch32(cond, scratch, rhs, label);

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.

fixed following:
  * replace masm.asmOnOutOfBoundsLabel() with wasm::JumpTarget::OutOfBounds

  * templatize
      * MacroAssemblerMIPSShared::ma_b
      * MacroAssemblerMIPSCompat::branchTestMagic
      * MacroAssemblerMIPSCompat::jump
      * MacroAssemblerMIPSCompat::branchTest32
      * MacroAssemblerMIPS64Compat::branchTestMagic
      * MacroAssemblerMIPS64Compat::jump
      * MacroAssemblerMIPS64Compat::branchTest32

  * add
      * MacroAssemblerMIPS::branchWithCode
      * MacroAssemblerMIPS64::branchWithCode

  * move MoveEmitterMIPSShared::MoveEmitterMIPSShared to cpp, as framePushed is not yet defined.

as AssemblerMIPSShared::bindLater is not-yet-implemented, MacroAssemblerMIPS::branchWithCode is also not-yet-implemented.

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Move MacroAssembler::branchTest64 into generic macro assembler.

branchTest64 was used in previous Math.random impl, and looks like it's no more used.
in this patch I moved it to MacroAssembler, but maybe it's better just remove it?

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Move MacroAssembler::branchTestPtr into generic macro assembler.

x86 and x64 have exactly same impl, so merged them in x86-shared.
also, mips32 and mips64 have exactly same impl, so merged them in mips-shared.

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.

in x86-shared, Operand variant seems not to be used, so removed.

mips32 and mips64 have exactly same impl, metged and applied template (but it's not buildable, as it does not support js::wasm::JumpTarget).

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Move MacroAssembler::branchFloat into generic macro assembler.

Comment 25

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8718233 [details] [diff] [review]
Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.

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

I will forward the review to our MIPS maintainers.

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Move MacroAssembler::branchTruncateFloat32 into generic macro assembler.

Comment 27

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Move MacroAssembler::branchDouble into generic macro assembler.

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Move MacroAssembler::branchTruncateDouble into generic macro assembler.

Comment 29

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8718234 [details] [diff] [review]
Part 4: Move MacroAssembler::branchTest64 into generic macro assembler.

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

::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +582,5 @@
> +    if (cond == Assembler::Zero) {
> +        MOZ_ASSERT(lhs.low == rhs.low);
> +        MOZ_ASSERT(lhs.high == rhs.high);
> +        mov(lhs.low, ScratchRegister);
> +        or32(lhs.high, ScratchRegister);

Note: Like mips, ARM has a 3 way or instruction:

  ma_orr(lhs.low, lhs.high, ScratchRegister);

Comment 30

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8718236 [details] [diff] [review]
Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.

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

::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +689,5 @@
> +MacroAssembler::branchTest32(Condition cond, const AbsoluteAddress& lhs, Imm32 rhs, Label* label)
> +{
> +    vixl::UseScratchRegisterScope temps(this);
> +    const Register scratch = temps.AcquireX().asUnsized();
> +    loadPtr(lhs, scratch);

=> ni? sstangl: Shouldn't this be a load32 instead of a loadPtr.

::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +350,5 @@
> +MacroAssembler::branchTest32(Condition cond, Register lhs, Register rhs, L label)
> +{
> +    MOZ_ASSERT(cond == Zero || cond == NonZero || cond == Signed || cond == NotSigned);
> +    if (lhs == rhs) {
> +        ma_b(lhs, rhs, label, cond);

This sounds busted, as this is the same implementation for branchTest64 on mips64.

=> ni? Heiher

Comment 31

[Nicolas B. Pierron [:nbp]]

Heiher, Sean, can you have a look at the review in comment 30?

Comment 32

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8718282 [details] [diff] [review]
Part 7: Move MacroAssembler::branchFloat into generic macro assembler.

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

Forward the review to Benjamin.

Comment 33

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8718283 [details] [diff] [review]
Part 8: Move MacroAssembler::branchTruncateFloat32 into generic macro assembler.

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

Forward the review to Benjamin.

Comment 34

[Sean Stangl [:sstangl]]

Comment on attachment 8718236 [details] [diff] [review]
Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.

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

::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +689,5 @@
> +MacroAssembler::branchTest32(Condition cond, const AbsoluteAddress& lhs, Imm32 rhs, Label* label)
> +{
> +    vixl::UseScratchRegisterScope temps(this);
> +    const Register scratch = temps.AcquireX().asUnsized();
> +    loadPtr(lhs, scratch);

Yes, this should be a load32.

Comment 35

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #30)
> Comment on attachment 8718236 [details] [diff] [review]
> Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.
> 
> Review of attachment 8718236 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
> @@ +350,5 @@
> > +MacroAssembler::branchTest32(Condition cond, Register lhs, Register rhs, L label)
> > +{
> > +    MOZ_ASSERT(cond == Zero || cond == NonZero || cond == Signed || cond == NotSigned);
> > +    if (lhs == rhs) {
> > +        ma_b(lhs, rhs, label, cond);
> 
> This sounds busted, as this is the same implementation for branchTest64 on
> mips64.
> 
> => ni? Heiher

I think yes, The 32-bit integers are always sign-extend into 64-bit register on mips64.

Comment 36

[Nicolas B. Pierron [:nbp]]

(In reply to Heiher [:hev] from comment #35)
> (In reply to Nicolas B. Pierron [:nbp] from comment #30)
> > Comment on attachment 8718236 [details] [diff] [review]
> > Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.
> > 
> > Review of attachment 8718236 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > ::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
> > @@ +350,5 @@
> > > +MacroAssembler::branchTest32(Condition cond, Register lhs, Register rhs, L label)
> > > +{
> > > +    MOZ_ASSERT(cond == Zero || cond == NonZero || cond == Signed || cond == NotSigned);
> > > +    if (lhs == rhs) {
> > > +        ma_b(lhs, rhs, label, cond);
> > 
> > This sounds busted, as this is the same implementation for branchTest64 on
> > mips64.
> > 
> > => ni? Heiher
> 
> I think yes, The 32-bit integers are always sign-extend into 64-bit register
> on mips64.

So that means that if we are trying to compare the low bits of a 64bits value, with a 32bits immediate, then we might have an issue where the test will not work as expected?

Should we neuter the high bits of the other register then?

Comment 37

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8718286 [details] [diff] [review]
Part 10: Move MacroAssembler::branchTruncateDouble into generic macro assembler.

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

::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +727,5 @@
> +    ARMRegister dest64(dest, 64);
> +
> +    MOZ_ASSERT(!scratch64.Is(dest64));
> +
> +    //breakpoint();

nit: remove this debug-comment.

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +211,5 @@
> +// was clamped to INT32_MIN/INT32_MAX, and we can test it.
> +// NOTE: if the value really was supposed to be INT32_MAX / INT32_MIN then it
> +// will be wrong.
> +void
> +MacroAssembler::branchTruncateDouble(FloatRegister src, Register dest, Label* fail)

This function is identical to MIPS32 implementation, move it into MacroAssembler-mips-shared-inl.h

Comment 38

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Move MacroAssembler::branchTruncateFloat32 into generic macro assembler.

merged mips32/64.

Comment 39

[Tooru Fujisawa [:arai]]

Attachment: Part 11: Move generic MacroAssembler methods into check_macroassembler_style block.

Moved following methods

  * branchIfFalseBool
  * branchIfTrueBool

  * branchIfRope
  * branchLatin1String
  * branchTwoByteString

  * branchIfFunctionHasNoScript
  * branchIfInterpreted
  * branchFunctionKind
  * branchIfNotInterpretedConstructor

  * branchTestObjClass
  * branchTestObjShape
  * branchTestObjGroup
  * branchTestObjectTruthy

  * branchTestClassIsProxy
  * branchTestObjectIsProxy
  * branchTestProxyHandlerFamily

  * branchTestMIRType
  * branchEqualTypeIfNeeded

  * branchKey
  * branchTestNeedsIncrementalBarrier

Comment 40

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #36)
> (In reply to Heiher [:hev] from comment #35)
> > (In reply to Nicolas B. Pierron [:nbp] from comment #30)
> > > Comment on attachment 8718236 [details] [diff] [review]
> > > Part 6: Move MacroAssembler::branchTest32 into generic macro assembler.
> > > 
> > > Review of attachment 8718236 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > >
> > > ::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
> > > @@ +350,5 @@
> > > > +MacroAssembler::branchTest32(Condition cond, Register lhs, Register rhs, L label)
> > > > +{
> > > > +    MOZ_ASSERT(cond == Zero || cond == NonZero || cond == Signed || cond == NotSigned);
> > > > +    if (lhs == rhs) {
> > > > +        ma_b(lhs, rhs, label, cond);
> > > 
> > > This sounds busted, as this is the same implementation for branchTest64 on
> > > mips64.
> > > 
> > > => ni? Heiher
> > 
> > I think yes, The 32-bit integers are always sign-extend into 64-bit register
> > on mips64.
> 
> So that means that if we are trying to compare the low bits of a 64bits
> value, with a 32bits immediate, then we might have an issue where the test
> will not work as expected?
> 
> Should we neuter the high bits of the other register then?

Yes. If one operand of branchTest32 is a real 64bits value, we need neuter the high bits of this value.

Comment 41

[hev [:hev]]

Comment on attachment 8718233 [details] [diff] [review]
Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.

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

Thank you fix, link failed:
Executing: mips64el-unknown-linux-gnu-g++ -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wtype-limits -Wcast-align -Wno-invalid-offsetof -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -g -O3 -march=loongson3a -fdiagnostics-color=auto -mno-branch-likely -w -fomit-frame-pointer -fPIC -shared -Wl,-z,defs -Wl,-h,libmozjs-47a1.so -o libmozjs-47a1.so /home/heiher/git/mips64/gecko-dev/js/src/mips64-release/js/src/tmpcr__PD.list -lpthread -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -Wl,-version-script,symverscript -Wl,-rpath-link,/home/heiher/git/mips64/gecko-dev/js/src/mips64-release/dist/bin -Wl,-rpath-link,/usr/lib -lm -ldl -L/opt/mips64el-toolchain/platforms/current/usr/lib64 -licui18n -licuuc -licudata -L/usr/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl -lz -lm
/home/heiher/git/mips64/gecko-dev/js/src/mips64-release/js/src/tmpcr__PD.list:
    INPUT("RegExp.o")
    INPUT("Parser.o")
    INPUT("StoreBuffer.o")
    INPUT("ExecutableAllocatorPosix.o")
    INPUT("jsarray.o")
    INPUT("jsatom.o")
    INPUT("jsdtoa.o")
    INPUT("jsmath.o")
    INPUT("jsutil.o")
    INPUT("pm_linux.o")
    INPUT("Initialization.o")
    INPUT("TraceLogging.o")
    INPUT("TraceLoggingGraph.o")
    INPUT("TraceLoggingTypes.o")
    INPUT("Unified_cpp_js_src0.o")
    INPUT("Unified_cpp_js_src1.o")
    INPUT("Unified_cpp_js_src10.o")
    INPUT("Unified_cpp_js_src11.o")
    INPUT("Unified_cpp_js_src12.o")
    INPUT("Unified_cpp_js_src13.o")
    INPUT("Unified_cpp_js_src14.o")
    INPUT("Unified_cpp_js_src15.o")
    INPUT("Unified_cpp_js_src16.o")
    INPUT("Unified_cpp_js_src17.o")
    INPUT("Unified_cpp_js_src18.o")
    INPUT("Unified_cpp_js_src19.o")
    INPUT("Unified_cpp_js_src2.o")
    INPUT("Unified_cpp_js_src20.o")
    INPUT("Unified_cpp_js_src21.o")
    INPUT("Unified_cpp_js_src22.o")
    INPUT("Unified_cpp_js_src23.o")
    INPUT("Unified_cpp_js_src24.o")
    INPUT("Unified_cpp_js_src25.o")
    INPUT("Unified_cpp_js_src26.o")
    INPUT("Unified_cpp_js_src27.o")
    INPUT("Unified_cpp_js_src28.o")
    INPUT("Unified_cpp_js_src29.o")
    INPUT("Unified_cpp_js_src3.o")
    INPUT("Unified_cpp_js_src30.o")
    INPUT("Unified_cpp_js_src31.o")
    INPUT("Unified_cpp_js_src32.o")
    INPUT("Unified_cpp_js_src33.o")
    INPUT("Unified_cpp_js_src34.o")
    INPUT("Unified_cpp_js_src35.o")
    INPUT("Unified_cpp_js_src36.o")
    INPUT("Unified_cpp_js_src4.o")
    INPUT("Unified_cpp_js_src5.o")
    INPUT("Unified_cpp_js_src6.o")
    INPUT("Unified_cpp_js_src7.o")
    INPUT("Unified_cpp_js_src8.o")
    INPUT("Unified_cpp_js_src9.o")

/opt/mips64el-toolchain/lib/gcc/mips64el-unknown-linux-gnu/5.3.0/../../../../mips64el-unknown-linux-gnu/bin/ld: skipping incompatible /usr/lib/libgcc_s.so when searching for -lgcc_s
/opt/mips64el-toolchain/lib/gcc/mips64el-unknown-linux-gnu/5.3.0/../../../../mips64el-unknown-linux-gnu/bin/ld: skipping incompatible /usr/lib/libgcc_s.so.1 when searching for libgcc_s.so.1
/opt/mips64el-toolchain/lib/gcc/mips64el-unknown-linux-gnu/5.3.0/../../../../mips64el-unknown-linux-gnu/bin/ld: skipping incompatible /usr/lib/libgcc_s.so when searching for -lgcc_s
/opt/mips64el-toolchain/lib/gcc/mips64el-unknown-linux-gnu/5.3.0/../../../../mips64el-unknown-linux-gnu/bin/ld: skipping incompatible /usr/lib/libgcc_s.so.1 when searching for libgcc_s.so.1
Unified_cpp_js_src0.o: In function `void js::jit::MacroAssemblerMIPS64Compat::jump<js::jit::Label*>(js::jit::Label*)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `void js::jit::MacroAssemblerMIPS64Compat::branchTest32<js::wasm::JumpTarget>(js::jit::AssemblerMIPSShared::Condition, js::jit::Register, js::jit::Register, js::wasm::JumpTarget)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `void js::jit::MacroAssemblerMIPS64Compat::jump<js::jit::Label*>(js::jit::Label*)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `void js::jit::MacroAssemblerMIPS64Compat::branchTest32<js::wasm::JumpTarget>(js::jit::AssemblerMIPSShared::Condition, js::jit::Register, js::jit::Register, js::wasm::JumpTarget)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `GenerateJitExitStub(js::wasm::ModuleGenerator&, unsigned int, js::wasm::ProfilingOffsets*) [clone .constprop.369]':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `void js::jit::MacroAssemblerMIPS64Compat::branchTest32<js::wasm::JumpTarget>(js::jit::AssemblerMIPSShared::Condition, js::jit::Register, js::jit::Register, js::wasm::JumpTarget)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `GenerateJitExitStub(js::wasm::ModuleGenerator&, unsigned int, js::wasm::ProfilingOffsets*) [clone .constprop.369]':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src1.o: In function `void js::jit::MacroAssemblerMIPS64Compat::branchTest32<js::wasm::JumpTarget>(js::jit::AssemblerMIPSShared::Condition, js::jit::Register, js::jit::Register, js::wasm::JumpTarget)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
Unified_cpp_js_src1.o:/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:537: more undefined references to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)' follow
Unified_cpp_js_src10.o: In function `void js::jit::MacroAssemblerMIPS64Compat::jump<js::jit::Label*>(js::jit::Label*)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src10.o: In function `js::jit::ICTableSwitch::Compiler::generateStubCode(js::jit::MacroAssembler&)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/BaselineIC.cpp:7656: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src10.o:/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: more undefined references to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)' follow
Unified_cpp_js_src11.o: In function `void js::jit::MacroAssemblerMIPS64Compat::branchTest32<js::wasm::JumpTarget>(js::jit::AssemblerMIPSShared::Condition, js::jit::Register, js::jit::Register, js::wasm::JumpTarget)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:540: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:540: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
Unified_cpp_js_src11.o: In function `void js::jit::MacroAssemblerMIPS64Compat::jump<js::jit::Label*>(js::jit::Label*)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
Unified_cpp_js_src11.o:/home/heiher/git/mips64/gecko-dev/js/src/jit/mips64/MacroAssembler-mips64.h:310: more undefined references to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)' follow
collect2: error: ld returned 1 exit status
make[3]: *** [libmozjs-47a1.so] Error 1
make[2]: *** [js/src/target] Error 2
make[1]: *** [compile] Error 2
make: *** [default] Error 2

Comment 42

[Tooru Fujisawa [:arai]]

(In reply to Heiher [:hev] from comment #40)
> Yes. If one operand of branchTest32 is a real 64bits value, we need neuter
> the high bits of this value.

it needs neutering only with `cond == Zero || cond == NonZero`, right?
iiuc, `cond == Signed || cond == NotSigned` needs highest bit.

Comment 43

[Tooru Fujisawa [:arai]]

(In reply to Heiher [:hev] from comment #41)
> Comment on attachment 8718233 [details] [diff] [review]
> Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.
> 
> Review of attachment 8718233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you fix, link failed:

Hmm, it works for me on osx.
Will try on linux.

Comment 44

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.

The link error is not reproducible with arm64 simulator build, both on osx (clang-700.1.81) and linux (gcc (Debian 4.9.2-10) 4.9.2).
Tested on m-c ac39fba33c6d revision.
Today's m-c has another changes from bug 1240583, and it's not buildable with this patch :/


Added `Label*` instantiation for ma_b as a possible fix for following:
>  undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'

but I'm not sure how to fix other one, as there is already `wasm::JumpTarget` instantiation:
> undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'

hev, can you take this over?

Comment 45

[hev [:hev]]

(In reply to Tooru Fujisawa [:arai] from comment #44)
> Created attachment 8719138 [details] [diff] [review]
> Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.
> 
> The link error is not reproducible with arm64 simulator build, both on osx
> (clang-700.1.81) and linux (gcc (Debian 4.9.2-10) 4.9.2).
> Tested on m-c ac39fba33c6d revision.
> Today's m-c has another changes from bug 1240583, and it's not buildable
> with this patch :/
> 
> 
> Added `Label*` instantiation for ma_b as a possible fix for following:
> >  undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::jit::Label*>(js::jit::Label*, js::jit::JumpKind)'
> 
> but I'm not sure how to fix other one, as there is already
> `wasm::JumpTarget` instantiation:
> > undefined reference to `void js::jit::MacroAssemblerMIPSShared::ma_b<js::wasm::JumpTarget>(js::jit::Register, js::jit::Register, js::wasm::JumpTarget, js::jit::AssemblerMIPSShared::Condition, js::jit::JumpKind)'
> 
> hev, can you take this over?

OK. Please wait, I'm on holiday.

Comment 46

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #44)
> Created attachment 8719138 [details] [diff] [review]
> Part 0: Fix MIPS32/MIPS64 jump related methods for wasm.
> 
> The link error is not reproducible with arm64 simulator build

sorry, I meant, mips64 simulator build.

Comment 47

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8718282 [details] [diff] [review]
Part 7: Move MacroAssembler::branchFloat into generic macro assembler.

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

Too bad you're not using MozReview for these code motions patches, it would make those trivial to review ;)
Thanks for doing this, though.

I'm pretty sure all platforms are doing the same things and a bit more refactoring could avoid platform specific code at all, but this is more work and outside the scope of this bug.

::: js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h
@@ +305,5 @@
> +        j(Parity, &unordered);
> +        j(Equal, label);
> +        bind(&unordered);
> +        return;
> +    }

Can you add a new line after this }, please?

Comment 48

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8719128 [details] [diff] [review]
Part 8: Move MacroAssembler::branchTruncateFloat32 into generic macro assembler.

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

Thank you for the patch.

Comment 49

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8719129 [details] [diff] [review]
Part 11: Move generic MacroAssembler methods into check_macroassembler_style block.

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

::: js/src/jit/MacroAssembler-inl.h
@@ +497,5 @@
> +        MOZ_CRASH("Bad MIRType");
> +    }
> +}
> +
> +template<typename T>

style-nit: extra space after template.

@@ +499,5 @@
> +}
> +
> +template<typename T>
> +void
> +MacroAssembler::branchKey(Condition cond, const T& length, const Int32Key& key, Label* label)

follow-up: Later, we should find a better name for Int32Key / branchKey.  And replace these arguments names by lhs / rhs.  It sounds to me that this function should just be named branch32.

::: js/src/jit/MacroAssembler.cpp
@@ +2507,5 @@
> +}
> +
> +void
> +MacroAssembler::branchEqualTypeIfNeeded(MIRType type, MDefinition* maybeDef, Register tag,
> +                                        Label* label)

follow-up: What do you think about renaming this function  maybeBranchTestType, or something similar?

Comment 50

[Tooru Fujisawa [:arai]]

thank you for reviewing :)

I'll land those patches for now, and will prepare remaining parts.
for mips, will file a dedicated bug.

Comment 51

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f95c21f4dd507563eef8e861ed3fbc5051f7df49
Bug 1245112 - Part 1: Move MacroAssembler::branchPrivatePtr into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/e92294275e905741fe4a2c90e91a698d5a4d468f
Bug 1245112 - Part 2: Move MacroAssembler::branchPtr into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9f675232a0f3576de34a460991f27d27a67589
Bug 1245112 - Part 3: Move MacroAssembler::branch32 into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/db95ea81a22cf467b27e34a11d15e5849129ecf4
Bug 1245112 - Part 4: Move MacroAssembler::branchTest64 into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/e179ad3903cc1dc80fcac91743beb955301bcf41
Bug 1245112 - Part 5: Move MacroAssembler::branchTestPtr into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb37e43c52e762a24a0f6368c012e1a96fd0e75b
Bug 1245112 - Part 6: Move MacroAssembler::branchTest32 into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/86e0dc5c462b1663b89a7561a072ab4930184fb8
Bug 1245112 - Part 7: Move MacroAssembler::branchFloat into generic macro assembler. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e12bd365f3b236759a2e9a5c26780b13ddf15a7
Bug 1245112 - Part 8: Move MacroAssembler::branchTruncateFloat32 into generic macro assembler. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d78e70a6fcc02161fabaedb91b869c92f4e6cc
Bug 1245112 - Part 9: Move MacroAssembler::branchDouble into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffdbc9bf8d81eeac3509de568e85a614a396d83b
Bug 1245112 - Part 10: Move MacroAssembler::branchTruncateDouble into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c49177aabb67d2c6468823476057997954d5cd0
Bug 1245112 - Part 11: Move generic MacroAssembler methods into check_macroassembler_style block. r=nbp

Comment 52

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f884ed3994b4c7e866f2cefd93b639f1bb6d655
Bug 1245112 - Part 11 followup: Add a space after template. r=bustage

Comment 53

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28f6cc5fa7c030346ba648a6d93d497f0b7334a7
Backed out changeset 7f884ed3994b (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/54d808e184fee32c9658718add02878c7ad016fc
Backed out changeset 6c49177aabb6 (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/be5c5019659ed6478b51b6d0906303c4fc2caa89
Backed out changeset ffdbc9bf8d81 (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5903ee04e8f4d09f83a1f9147bc7d8e088e669
Backed out changeset f8d78e70a6fc (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/22a9a917638aa330c87448fefe6987e74bd3ebc5
Backed out changeset 4e12bd365f3b (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/d542d2a8eda1fc2d6359d1857651e701367b21ec
Backed out changeset 86e0dc5c462b (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/743285bf168063684a2f6bd18dba93bb0413ea42
Backed out changeset cb37e43c52e7 (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e95e4d405ef7b9927db7285bdac500e8dcb46ac
Backed out changeset e179ad3903cc (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e817588b35454ee21f6e2d3f95ce624b855f46
Backed out changeset db95ea81a22c (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/46bcabfc04c8fc1a2f6c17a12d0ffec54c678ca2
Backed out changeset 3d9f675232a0 (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/50279e2f620d40800a925d12322e824ed58db909
Backed out changeset e92294275e90 (bug 1245112)

https://hg.mozilla.org/integration/mozilla-inbound/rev/3927e5cf68a2b9e9cb816aa30dc4e985a21c6b41
Backed out changeset f95c21f4dd50 (bug 1245112)

Comment 54

[Tooru Fujisawa [:arai]]

There is win64 specific issue with branchTest32 and Operand.
> c:/builds/moz2_slave/m-in-w64-000000000000000000000/build/src/js/src/jit/x64/Trampoline-x64.cpp(97) : error C2664:
> 'void js::jit::MacroAssembler::branchTest32(js::jit::AssemblerX86Shared::Condition,const js::jit::AbsoluteAddress
> &,js::jit::Imm32,js::jit::Label *)' : cannot convert argument 2 from 'const js::jit::Operand' to 'js::jit::Register'

will try fixing it.

Comment 55

[Tooru Fujisawa [:arai]]

Attachment: Part 6 followup

So I overlooked this single consumer of Operand variant of branchTest32.
it could just be replaced by Address.

Comment 56

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8719587 [details] [diff] [review]
Part 6 followup

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

\o/

Comment 57

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae0d9254da420acea359d6c5160b07ae5b28f99
Bug 1245112 - Part 1: Move MacroAssembler::branchPrivatePtr into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/229cf45c17d5e08b6c7126f0e8fdd8feccad522d
Bug 1245112 - Part 2: Move MacroAssembler::branchPtr into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/44927d05b68566a70753227ba36e47051bc4092d
Bug 1245112 - Part 3: Move MacroAssembler::branch32 into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/279bdd1c241f16270add8d3cf7bc27159083dc12
Bug 1245112 - Part 4: Move MacroAssembler::branchTest64 into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/50fc6edd4906bfebdf4dc932a634a281850eadb6
Bug 1245112 - Part 5: Move MacroAssembler::branchTestPtr into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/f433eabad0eecdaf0d895e68fae7f3501dd52c04
Bug 1245112 - Part 6: Move MacroAssembler::branchTest32 into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/38551974573c53bf0f4d47eb18c1dd56c1d3a734
Bug 1245112 - Part 7: Move MacroAssembler::branchFloat into generic macro assembler. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/97eff9ac5a850a73cd62cfc231aa3be3fc8e9d5b
Bug 1245112 - Part 8: Move MacroAssembler::branchTruncateFloat32 into generic macro assembler. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/88cc3e1cfb3b8e85166370bf3fac769e4bffe454
Bug 1245112 - Part 9: Move MacroAssembler::branchDouble into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb46f0c6477dc7b142a4e83633666d49b2784c9a
Bug 1245112 - Part 10: Move MacroAssembler::branchTruncateDouble into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4383349c6f4a57cd8ef8fc55de8cbdc215b63c
Bug 1245112 - Part 11: Move generic MacroAssembler methods into check_macroassembler_style block. r=nbp

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ae0d9254da4
https://hg.mozilla.org/mozilla-central/rev/229cf45c17d5
https://hg.mozilla.org/mozilla-central/rev/44927d05b685
https://hg.mozilla.org/mozilla-central/rev/279bdd1c241f
https://hg.mozilla.org/mozilla-central/rev/50fc6edd4906
https://hg.mozilla.org/mozilla-central/rev/f433eabad0ee
https://hg.mozilla.org/mozilla-central/rev/38551974573c
https://hg.mozilla.org/mozilla-central/rev/97eff9ac5a85
https://hg.mozilla.org/mozilla-central/rev/88cc3e1cfb3b
https://hg.mozilla.org/mozilla-central/rev/fb46f0c6477d
https://hg.mozilla.org/mozilla-central/rev/9b4383349c6f

Comment 59

[Tooru Fujisawa [:arai]]

Attachment: Part 12: Move MacroAssembler::branchPtrWithPatch into generic macro assembler.

Comment 60

[Tooru Fujisawa [:arai]]

Attachment: Part 13: Move MacroAssembler::branchPtrInNurseryRange into generic macro assembler.

Comment 61

[Tooru Fujisawa [:arai]]

Attachment: Part 14: Move MacroAssembler::branchValueIsNurseryObject into generic macro assembler.

Comment 62

[Tooru Fujisawa [:arai]]

Attachment: Part 15: Move MacroAssembler::branchAdd32 into generic macro assembler.

Comment 63

[Tooru Fujisawa [:arai]]

Attachment: Part 16: Move MacroAssembler::branchSub32 into generic macro assembler.

Comment 64

[Tooru Fujisawa [:arai]]

Attachment: Part 17: Move MacroAssembler::decBranchPtr into generic macro assembler.

Comment 65

[Tooru Fujisawa [:arai]]

Attachment: Part 18: Move MacroAssembler::branchTestInt32 into generic macro assembler.

Comment 66

[Tooru Fujisawa [:arai]]

Attachment: Part 19: Move MacroAssembler::branchTestInt32Truthy into generic macro assembler.

Comment 67

[Lars T Hansen [:lth]]

Comment on attachment 8721746 [details] [diff] [review]
Part 15: Move MacroAssembler::branchAdd32 into generic macro assembler.

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

Yes please.

Comment 68

[Jan de Mooij [:jandem]]

Comment on attachment 8721749 [details] [diff] [review]
Part 18: Move MacroAssembler::branchTestInt32 into generic macro assembler.

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

::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +772,5 @@
>  
> +void
> +MacroAssembler::branchTestInt32(Condition cond, Register tag, Label* label)
> +{
> +    return branchTestInt32Impl(cond, tag, label);

Nit: it's a void function so you could remove the |return| here and below, same for some of the other files.

Comment 69

[Jan de Mooij [:jandem]]

Comment on attachment 8721750 [details] [diff] [review]
Part 19: Move MacroAssembler::branchTestInt32Truthy into generic macro assembler.

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

Thanks.

Comment 70

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8721743 [details] [diff] [review]
Part 12: Move MacroAssembler::branchPtrWithPatch into generic macro assembler.

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

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +545,5 @@
>          return jumpWithPatch(label);
>      }
>  
>      template <typename S, typename T>
> +    CodeOffsetJump branchPtrWithPatchImpl(Condition cond, S lhs, T rhs, RepatchLabel* label) {

I do not see the benefit of an *Impl function for this tiny amount of code, removing it would have a diffstat of +4-7.

@@ +551,1 @@
>          return jumpWithPatch(label, cond);

This implementation will work on both x86 and x64.  Move it to x86-shared and use the PER_SHARED_ARCH macro.

Comment 71

[Tooru Fujisawa [:arai]]

Thank you for reviewing! :D

Comment 72

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1a49cc5a331fadfd6fb43f391d0d108263038a
Bug 1245112 - Part 12: Move MacroAssembler::branchPtrWithPatch into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2411ecdbc3aa5d1bfc1997c8cdde5229ef37bc9
Bug 1245112 - Part 13: Move MacroAssembler::branchPtrInNurseryRange into generic macro assembler. r=jonco

https://hg.mozilla.org/integration/mozilla-inbound/rev/5db0c4768aec4597bb0a3dc06ab72a51bb29e2a4
Bug 1245112 - Part 14: Move MacroAssembler::branchValueIsNurseryObject into generic macro assembler. r=jonco

https://hg.mozilla.org/integration/mozilla-inbound/rev/52aa480e7626544366090fc7bca0fd17f0168b19
Bug 1245112 - Part 15: Move MacroAssembler::branchAdd32 into generic macro assembler. r=lth

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f1c65dee0cb4fc921ce0cf05f7499c2a02f4b62
Bug 1245112 - Part 16: Move MacroAssembler::branchSub32 into generic macro assembler. r=lth

https://hg.mozilla.org/integration/mozilla-inbound/rev/df1a273493d59f3c9cf4d9278b6d799ddfb94f30
Bug 1245112 - Part 17: Move MacroAssembler::decBranchPtr into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/865173b1d24a5acb5e482c07584497a076f8cccc
Bug 1245112 - Part 18: Move MacroAssembler::branchTestInt32 into generic macro assembler. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/af10b351a596618049cab6c3d0acd131ea07d288
Bug 1245112 - Part 19: Move MacroAssembler::branchTestInt32Truthy into generic macro assembler. r=jandem

Comment 73

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

https://hg.mozilla.org/mozilla-central/rev/5b1a49cc5a33
https://hg.mozilla.org/mozilla-central/rev/c2411ecdbc3a
https://hg.mozilla.org/mozilla-central/rev/5db0c4768aec
https://hg.mozilla.org/mozilla-central/rev/52aa480e7626
https://hg.mozilla.org/mozilla-central/rev/2f1c65dee0cb
https://hg.mozilla.org/mozilla-central/rev/df1a273493d5
https://hg.mozilla.org/mozilla-central/rev/865173b1d24a
https://hg.mozilla.org/mozilla-central/rev/af10b351a596

Comment 74

[Tooru Fujisawa [:arai]]

Attachment: Part 18 followup: More refactor branchTestInt32.

Prepared remaining parts.

Even after applying all these patches, there are a lot of code duplications, such like a bunch of branchTest_TYPE_Impl() calls in branchTest_TYPE_() method, copied across x86-shared, arm, and arm64 archs.

The main issue is that mips32/mips64 doesn't have test_TYPE_() methods that returns Condition, as their branch instruction needs InstImm, not Condition.  Otherwise we can merge most of implementations into single template method in MacroAssembler-inl.h, or MacroAssembler-nunbox32-inl.h/MacroAssembler-punbox64-inl.h mentioned in bug 996602.

So, I'd like to treat this bug as a preparation for further refactoring, and leave those dupliacations for now.


[Part 18 followup]

while moving other Value related methods, I noticed there are some more space for cleanup/refactoring in branchTestInt32, like aligning variable name with others, delegating branch code to Register variant, and removing redundant assertions.

In x64, only Address variant uses different code than original testInt32, that calls cmp32 directly,

https://dxr.mozilla.org/mozilla-central/rev/5b2baa5e9356644a7ed0b73e422eaff62e159ffb/js/src/jit/x64/MacroAssembler-x64.h#727
>     void branchTestInt32Impl(Condition cond, const Operand& operand, Label* label) {
>         MOZ_ASSERT(cond == Equal || cond == NotEqual);
>         cmp32(ToUpper32(operand), Imm32(Upper32Of(GetShiftedTag(JSVAL_TYPE_INT32))));
>         j(cond, label);
>     }

iiuc, it's better than testInt32 impl, as it does not require scratch register, right?

>     Condition testInt32(Condition cond, const Address& src) {
>         ScratchRegisterScope scratch(asMasm());
>         splitTag(src, scratch);
>         return testInt32(cond, scratch);
>     }

changed testInt32 impl to use cmp32, and merged all x86 and x64 methods with testInt32+j.

Comment 75

[Tooru Fujisawa [:arai]]

Attachment: Part 20: Move MacroAssembler::branchTestDouble into generic macro assembler.

in remaining parts, they mostly follow the style as branchTestInt32, updated in Part 18 followup.

Comment 76

[Tooru Fujisawa [:arai]]

Attachment: Part 21: Move MacroAssembler::branchTestDoubleTruthy into generic macro assembler.

Will ask review for other parts after getting review for parts 18.1, 20 and 21, as fix required there should also be applied to others.

Comment 77

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8723136 [details] [diff] [review]
Part 18 followup: More refactor branchTestInt32.

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

::: js/src/jit/MacroAssembler.h
@@ +917,5 @@
>      inline void branchKey(Condition cond, const T& length, const Int32Key& key, Label* label);
>  
>      inline void branchTestNeedsIncrementalBarrier(Condition cond, Label* label);
>  
> +    inline void branchTestInt32(Condition cond, Register tag, Label* label) PER_SHARED_ARCH;

nit: This variant of the function deserve a comment as it is different than the 4 others.  The 4 other functions named branchTestInt32 are testing with either a Value or the address of a Value.  As opposed to these function, this one check the register which contains the tag (32bits boxing), or the tagged value (64bits boxing).

@@ +926,2 @@
>  
> +    inline void branchTestInt32Truthy(bool truthy, const ValueOperand& value, Label* label)

nit: Add a comment above this function to describe how it is different than the previous one.

Comment 78

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8723141 [details] [diff] [review]
Part 20: Move MacroAssembler::branchTestDouble into generic macro assembler.

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

::: js/src/jit/MacroAssembler.h
@@ +926,5 @@
>  
>      inline void branchTestInt32Truthy(bool truthy, const ValueOperand& value, Label* label)
>          DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
>  
> +    inline void branchTestDouble(Condition cond, Register tag, Label* label)

Do the same as commented for branchTestInt32, and also for the following patches.

Comment 79

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8723144 [details] [diff] [review]
Part 21: Move MacroAssembler::branchTestDoubleTruthy into generic macro assembler.

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

::: js/src/jit/MacroAssembler.h
@@ +933,5 @@
>      inline void branchTestDouble(Condition cond, const BaseIndex& address, Label* label) PER_SHARED_ARCH;
>      inline void branchTestDouble(Condition cond, const ValueOperand& value, Label* label)
>          DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
>  
> +    inline void branchTestDoubleTruthy(bool truthy, FloatRegister reg, Label* label) PER_SHARED_ARCH;

nit: add a comment which describe what "Truthy" means.

Comment 80

[Tooru Fujisawa [:arai]]

Attachment: Part 18 followup: More refactor branchTestInt32.

Added comments and private *Impl method.
can you review those parts?

will add each branchTest* methods to each comment sections, like following:

>     // Perform a type-test on a tag of a Value (32bits boxing), or the tagged
>     // value (64bits boxing).
>     inline void branchTestInt32(Condition cond, Register tag, Label* label) PER_SHARED_ARCH;
>     inline void branchTestDouble(Condition cond, Register tag, Label* label)
>         DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
> 
>     // Perform a type-test on a Value, addressed by Address or BaseIndex, or
>     // loaded into ValueOperand.
>     inline void branchTestInt32(Condition cond, const Address& address, Label* label) PER_SHARED_ARCH;
>     inline void branchTestInt32(Condition cond, const BaseIndex& address, Label* label) PER_SHARED_ARCH;
>     inline void branchTestInt32(Condition cond, const ValueOperand& value, Label* label)
>         DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
> 
>     inline void branchTestDouble(Condition cond, const Address& address, Label* label) PER_SHARED_ARCH;
>     inline void branchTestDouble(Condition cond, const BaseIndex& address, Label* label) PER_SHARED_ARCH;
>     inline void branchTestDouble(Condition cond, const ValueOperand& value, Label* label)
>         DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
> 
>     // Checks if given Value is evaluated to true or false in a condition.
>     // Type of the value should match to the type of the method.
>     inline void branchTestInt32Truthy(bool truthy, const ValueOperand& value, Label* label)
>         DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
>     inline void branchTestDoubleTruthy(bool truthy, FloatRegister reg, Label* label) PER_SHARED_ARCH;
> 
>   private:
> 
>     // Implementation for branchTest* methods.
>     template <typename T>
>     inline void branchTestInt32Impl(Condition cond, const T& t, Label* label)
>         DEFINED_ON(arm, arm64, x86_shared);
>     template <typename T>
>     inline void branchTestDoubleImpl(Condition cond, const T& t, Label* label)
>         DEFINED_ON(arm, arm64, x86_shared);
> 
>     //}}} check_macroassembler_style

Comment 81

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8723669 [details] [diff] [review]
Part 18 followup: More refactor branchTestInt32.

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

::: js/src/jit/MacroAssembler.h
@@ +930,4 @@
>          DEFINED_ON(arm, arm64, mips32, mips64, x86_shared);
>  
> +    // Checks if given Value is evaluated to true or false in a condition.
> +    // Type of the value should match to the type of the method.

nit: The type of the value should match the type of the method.

Comment 82

[Tooru Fujisawa [:arai]]

Attachment: Part 22: Move MacroAssembler::branchTestNumber into generic macro assembler.

here're remaining parts, that does almost same thing as Int32 and Double, for each type

Comment 83

[Tooru Fujisawa [:arai]]

Attachment: Part 23: Move MacroAssembler::branchTestBoolean into generic macro assembler.

Comment 84

[Tooru Fujisawa [:arai]]

Attachment: Part 24: Move MacroAssembler::branchTestBooleanTruthy into generic macro assembler.

Comment 85

[Tooru Fujisawa [:arai]]

Attachment: Part 25: Move MacroAssembler::branchTestUndefined into generic macro assembler.

Comment 86

[Tooru Fujisawa [:arai]]

Attachment: Part 26: Move MacroAssembler::branchTestString into generic macro assembler.

Comment 87

[Tooru Fujisawa [:arai]]

Attachment: Part 27: Move MacroAssembler::branchTestStringTruthy into generic macro assembler.

Comment 88

[Tooru Fujisawa [:arai]]

Attachment: Part 28: Move MacroAssembler::branchTestSymbol into generic macro assembler.

Comment 89

[Tooru Fujisawa [:arai]]

Attachment: Part 29: Move MacroAssembler::branchTestNull into generic macro assembler.

Comment 90

[Tooru Fujisawa [:arai]]

Attachment: Part 30: Move MacroAssembler::branchTestObject into generic macro assembler.

Comment 91

[Tooru Fujisawa [:arai]]

Attachment: Part 31: Move MacroAssembler::branchTestGCThing into generic macro assembler.

Comment 92

[Tooru Fujisawa [:arai]]

Attachment: Part 32: Move MacroAssembler::branchTestPrimitive into generic macro assembler.

Comment 93

[Tooru Fujisawa [:arai]]

Attachment: Part 33: Move MacroAssembler::branchTestMagic into generic macro assembler.

Comment 94

[Tooru Fujisawa [:arai]]

Attachment: Part 34: Move MacroAssembler::branchTestMagicValue into generic macro assembler.

Comment 95

[Tooru Fujisawa [:arai]]

Attachment: Part 35: Move MacroAssembler::branchTestValue into generic macro assembler.

Address variant of branchTestValue is not used, so removed.

Comment 96

[Tooru Fujisawa [:arai]]

Attachment: Part 36: Move MacroAssembler::branchPtrImpl into generic macro assembler.

Part 2 added branchPtrImpl to x86/x64.  moved them to genric MacroAssembler.

Comment 97

[Tooru Fujisawa [:arai]]

Attachment: Part 33: Move MacroAssembler::branchTestMagic into generic macro assembler.

fixed branchTestMagicImpl template for arm64.

Comment 98

[Jan de Mooij [:jandem]]

Comment on attachment 8724238 [details] [diff] [review]
Part 34: Move MacroAssembler::branchTestMagicValue into generic macro assembler.

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

Excellent work, arai.

Comment 99

[Jan de Mooij [:jandem]]

Comment on attachment 8724241 [details] [diff] [review]
Part 35: Move MacroAssembler::branchTestValue into generic macro assembler.

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

::: js/src/jit/x86/MacroAssembler-x86-inl.h
@@ +386,5 @@
>      j(truthy ? NonZero : Zero, label);
>  }
>  
> +void
> +MacroAssembler::branchTestValue(Condition cond, const ValueOperand& lhs,

Since the x86/arm/mips32 implementations are pretty long (the x86 version is at least 15 lines) I think it'd make sense to keep branchTestValue in the cpp files. What do you think?

Comment 100

[Sean Stangl [:sstangl]]

Comment on attachment 8724225 [details] [diff] [review]
Part 23: Move MacroAssembler::branchTestBoolean into generic macro assembler.

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

::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +569,5 @@
> +
> +void
> +MacroAssembler::branchTestBoolean(Condition cond, const Address& address, Label* label)
> +{
> +    extractTag(address, SecondScratchReg);

Please don't hardcode SecondScratchReg: instead, use a ScratchRegisterScope.

@@ +576,5 @@
> +
> +void
> +MacroAssembler::branchTestBoolean(Condition cond, const BaseIndex& address, Label* label)
> +{
> +    extractTag(address, SecondScratchReg);

ScratchRegisterScope.

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +262,5 @@
>  
> +void
> +MacroAssembler::branchTestBoolean(Condition cond, const ValueOperand& value, Label* label)
> +{
> +    splitTag(value, SecondScratchReg);

Does MIPS64 have a ScratchRegsiterScope? MIPS32 does. If it does, please use that instead.

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +332,5 @@
>          return cond;
>      }
>      Condition testBoolean(Condition cond, const Address& src) {
> +        cmp32(ToUpper32(src), Imm32(Upper32Of(GetShiftedTag(JSVAL_TYPE_BOOLEAN))));
> +        return cond;

Nice.

Comment 101

[Sean Stangl [:sstangl]]

Comment on attachment 8724226 [details] [diff] [review]
Part 24: Move MacroAssembler::branchTestBooleanTruthy into generic macro assembler.

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

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +269,5 @@
>  
> +void
> +MacroAssembler::branchTestBooleanTruthy(bool b, const ValueOperand& value, Label* label)
> +{
> +    unboxBoolean(value, SecondScratchReg);

Please use a ScratchRegisterScope. (I know the original code didn't... argh.)

Comment 102

[Sean Stangl [:sstangl]]

Comment on attachment 8724227 [details] [diff] [review]
Part 25: Move MacroAssembler::branchTestUndefined into generic macro assembler.

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

::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +518,5 @@
> +
> +void
> +MacroAssembler::branchTestUndefined(Condition cond, const Address& address, Label* label)
> +{
> +    extractTag(address, SecondScratchReg);

ScratchRegisterScope?

@@ +525,5 @@
> +
> +void
> +MacroAssembler::branchTestUndefined(Condition cond, const BaseIndex& address, Label* label)
> +{
> +    extractTag(address, SecondScratchReg);

ScratchRegisterScope?

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +227,5 @@
>  void
> +MacroAssembler::branchTestUndefined(Condition cond, const ValueOperand& value,
> +                                                Label* label)
> +{
> +    splitTag(value, SecondScratchReg);

ScratchRegisterScope.

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +323,5 @@
>  
>  
>      Condition testUndefined(Condition cond, const Address& src) {
> +        cmp32(ToUpper32(src), Imm32(Upper32Of(GetShiftedTag(JSVAL_TYPE_UNDEFINED))));
> +        return cond;

Nice again.

Comment 103

[Sean Stangl [:sstangl]]

Ahem, those above should read "SecondScratchRegisterScope".

Comment 104

[Sean Stangl [:sstangl]]

Comment on attachment 8724228 [details] [diff] [review]
Part 26: Move MacroAssembler::branchTestString into generic macro assembler.

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

::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +611,5 @@
> +
> +void
> +MacroAssembler::branchTestString(Condition cond, const BaseIndex& address, Label* label)
> +{
> +    extractTag(address, SecondScratchReg);

SecondScratchRegisterScope

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +284,5 @@
>  
> +void
> +MacroAssembler::branchTestString(Condition cond, const ValueOperand& value, Label* label)
> +{
> +    splitTag(value, SecondScratchReg);

SecondScratchRegisterScope

Comment 105

[Sean Stangl [:sstangl]]

Comment on attachment 8724229 [details] [diff] [review]
Part 27: Move MacroAssembler::branchTestStringTruthy into generic macro assembler.

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

::: js/src/jit/mips32/MacroAssembler-mips32-inl.h
@@ +349,5 @@
> +void
> +MacroAssembler::branchTestStringTruthy(bool b, const ValueOperand& value, Label* label)
> +{
> +    Register string = value.payloadReg();
> +    ma_lw(SecondScratchReg, Address(string, JSString::offsetOfLength()));

SecondScratchRegisterScope

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +291,5 @@
>  
> +void
> +MacroAssembler::branchTestStringTruthy(bool b, const ValueOperand& value, Label* label)
> +{
> +    unboxString(value, SecondScratchReg);

SecondScratchRegisterScope

Comment 106

[Sean Stangl [:sstangl]]

Comment on attachment 8724232 [details] [diff] [review]
Part 29: Move MacroAssembler::branchTestNull into generic macro assembler.

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

::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +639,5 @@
> +
> +void
> +MacroAssembler::branchTestNull(Condition cond, const Address& address, Label* label)
> +{
> +    extractTag(address, SecondScratchReg);

SecondScratchRegisterScope

@@ +646,5 @@
> +
> +void
> +MacroAssembler::branchTestNull(Condition cond, const BaseIndex& address, Label* label)
> +{
> +    extractTag(address, SecondScratchReg);

SecondScratchRegisterScope

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +306,5 @@
>  
> +void
> +MacroAssembler::branchTestNull(Condition cond, const ValueOperand& value, Label* label)
> +{
> +    splitTag(value, SecondScratchReg);

SecondScratchRegisterScope

Comment 107

[Sean Stangl [:sstangl]]

Comment on attachment 8724233 [details] [diff] [review]
Part 30: Move MacroAssembler::branchTestObject into generic macro assembler.

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

::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +660,5 @@
> +
> +void
> +MacroAssembler::branchTestObject(Condition cond, const Address& address, Label* label)
> +{
> +    extractTag(address, SecondScratchReg);

SecondScratchRegisterScope

@@ +667,5 @@
> +
> +void
> +MacroAssembler::branchTestObject(Condition cond, const BaseIndex& address, Label* label)
> +{
> +    extractTag(address, SecondScratchReg);

SecondScratchRegisterScope

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +313,5 @@
>  
> +void
> +MacroAssembler::branchTestObject(Condition cond, const ValueOperand& value, Label* label)
> +{
> +    splitTag(value, SecondScratchReg);

SecondScratchRegisterScope

Comment 108

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Jan de Mooij [:jandem] from comment #99)
> Comment on attachment 8724241 [details] [diff] [review]
> Part 35: Move MacroAssembler::branchTestValue into generic macro assembler.
> 
> Review of attachment 8724241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/x86/MacroAssembler-x86-inl.h
> @@ +386,5 @@
> >      j(truthy ? NonZero : Zero, label);
> >  }
> >  
> > +void
> > +MacroAssembler::branchTestValue(Condition cond, const ValueOperand& lhs,
> 
> Since the x86/arm/mips32 implementations are pretty long (the x86 version is
> at least 15 lines) I think it'd make sense to keep branchTestValue in the
> cpp files. What do you think?

I agree, moved to cpp locally.


(In reply to Sean Stangl [:sstangl] from comment #100)
> Comment on attachment 8724225 [details] [diff] [review]
> Part 23: Move MacroAssembler::branchTestBoolean into generic macro assembler.
> 
> Review of attachment 8724225 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
> @@ +569,5 @@
> > +
> > +void
> > +MacroAssembler::branchTestBoolean(Condition cond, const Address& address, Label* label)
> > +{
> > +    extractTag(address, SecondScratchReg);
> 
> Please don't hardcode SecondScratchReg: instead, use a ScratchRegisterScope.

Applied to all other not-yet-landed patches too.
for already-landed patches, will post followup.

Comment 109

[Tooru Fujisawa [:arai]]

thank you!

there is already bug 1205524 for ScratchRegisterScope on mips.
will land remaining patches shortly.

Comment 110

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65554e19955c8a610addf7d05e7fa889e53246ca
Bug 1245112 - Part 18 followup: More refactor branchTestInt32. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8bfd44ff31694b1c72c1bf87ce65098310af710
Bug 1245112 - Part 20: Move MacroAssembler::branchTestDouble into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/63f26d3fe989ec5ed10950328d68c957bac9990d
Bug 1245112 - Part 21: Move MacroAssembler::branchTestDoubleTruthy into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/6db0c9d5144e0fc9cd98cd33a440ac970ab008f0
Bug 1245112 - Part 22: Move MacroAssembler::branchTestNumber into generic macro assembler. r=bhackett

https://hg.mozilla.org/integration/mozilla-inbound/rev/e28770d913990481aa6e5054924d6f6a2023b470
Bug 1245112 - Part 23: Move MacroAssembler::branchTestBoolean into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/53abfbd00384ab2656296de21621ff093aab90db
Bug 1245112 - Part 24: Move MacroAssembler::branchTestBooleanTruthy into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7cc18bce598c3f843c0ed5e7ad582042629971d
Bug 1245112 - Part 25: Move MacroAssembler::branchTestUndefined into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/0915a28459b5e4550ffe3b9e62775c47435f19c7
Bug 1245112 - Part 26: Move MacroAssembler::branchTestString into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/067c628f3135648e001eb10bf34fe1de505d1511
Bug 1245112 - Part 27: Move MacroAssembler::branchTestStringTruthy into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/18a49e013b77aa86ae8711b4d69362141fe6e54f
Bug 1245112 - Part 28: Move MacroAssembler::branchTestSymbol into generic macro assembler. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e20d12317e31fa07fd6f1490da6c2b92baa5ca4
Bug 1245112 - Part 29: Move MacroAssembler::branchTestNull into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bac6341260b00cbac90fa49390ebec01a50b662
Bug 1245112 - Part 30: Move MacroAssembler::branchTestObject into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/d95aba27394f2d07475b1ed2b8eb12083c1c23d9
Bug 1245112 - Part 31: Move MacroAssembler::branchTestGCThing into generic macro assembler. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/444cfe1bcbc76fc29d7c6adf85f7225a2f37a837
Bug 1245112 - Part 32: Move MacroAssembler::branchTestPrimitive into generic macro assembler. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab4ba99b59e865c98a17bede6f3085c2fc5592b0
Bug 1245112 - Part 33: Move MacroAssembler::branchTestMagic into generic macro assembler. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dee595021cd49d155cd7e123fd7741a3657c021
Bug 1245112 - Part 34: Move MacroAssembler::branchTestMagicValue into generic macro assembler. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/81a2e1207697113d12dda88a959a7d1768309dc4
Bug 1245112 - Part 35: Move MacroAssembler::branchTestValue into generic macro assembler. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/7889f529f6c83d3c90f83ab053cf062057ddbeed
Bug 1245112 - Part 36: Move MacroAssembler::branchPtrImpl into generic macro assembler. r=nbp

Comment 111

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65554e19955c
https://hg.mozilla.org/mozilla-central/rev/e8bfd44ff316
https://hg.mozilla.org/mozilla-central/rev/63f26d3fe989
https://hg.mozilla.org/mozilla-central/rev/6db0c9d5144e
https://hg.mozilla.org/mozilla-central/rev/e28770d91399
https://hg.mozilla.org/mozilla-central/rev/53abfbd00384
https://hg.mozilla.org/mozilla-central/rev/d7cc18bce598
https://hg.mozilla.org/mozilla-central/rev/0915a28459b5
https://hg.mozilla.org/mozilla-central/rev/067c628f3135
https://hg.mozilla.org/mozilla-central/rev/18a49e013b77
https://hg.mozilla.org/mozilla-central/rev/4e20d12317e3
https://hg.mozilla.org/mozilla-central/rev/6bac6341260b
https://hg.mozilla.org/mozilla-central/rev/d95aba27394f
https://hg.mozilla.org/mozilla-central/rev/444cfe1bcbc7
https://hg.mozilla.org/mozilla-central/rev/ab4ba99b59e8
https://hg.mozilla.org/mozilla-central/rev/3dee595021cd
https://hg.mozilla.org/mozilla-central/rev/81a2e1207697
https://hg.mozilla.org/mozilla-central/rev/7889f529f6c8