1229057 - Move MacroAssembler arithmetic functions to the generic macro assembler.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Tooru Fujisawa [:arai]]

just like bug 1203964, Move declarations of following to the generic MacroAssembler:
  * add32
  * addPtr
  * add64
  * addFloat32
  * addConstantFloat32
  * addDouble
  * addConstantDouble

  * sub32
  * subPtr
  * subDouble

  * mul32
  * mul64
  * mulDouble
  * mulDoublePtr
  * mulBy3

  * divlDouble

  * inc64

If the list is wrong, please tell me :)

Comment 1

[Lars T Hansen [:lth]]

sub32 was moved last week (to support bug 1108290).

Comment 2

[Tooru Fujisawa [:arai]]

oh, right
thanks :)

Comment 3

[Tooru Fujisawa [:arai]]

looks like following are also arithmetic functions:
  * neg32
  * negateFloat
  * negateDouble

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Move MacroAssembler::addPtr into generic macro assembler

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8694038 [details] [diff] [review]
Part 1: Move MacroAssembler::addPtr into generic macro assembler

prepared 19 patches

green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75dd3ee0cdc7

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Move MacroAssembler::add32 into generic macro assembler

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Move MacroAssembler::add64 into generic macro assembler

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Move MacroAssembler::addFloat32 into generic macro assembler

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Move MacroAssembler::addConstantFloat32 into generic macro assembler

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Move MacroAssembler::addDouble into generic macro assembler

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Move MacroAssembler::addConstantDouble into generic macro assembler

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Move MacroAssembler::subPtr into generic macro assembler

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Move MacroAssembler::subDouble into generic macro assembler

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Move MacroAssembler::mul32 into generic macro assembler

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 11: Move MacroAssembler::mul64 into generic macro assembler

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 12: Move MacroAssembler::mulBy3 into generic macro assembler

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 13: Move MacroAssembler::mulDoublePtr into generic macro assembler

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part 14: Move MacroAssembler::mulDouble into generic macro assembler

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 15: Move MacroAssembler::divDouble into generic macro assembler

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 16: Move MacroAssembler::inc64 into generic macro assembler

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Part 17: Move MacroAssembler::neg32 into generic macro assembler

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Part 18: Move MacroAssembler::negateFloat into generic macro assembler

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 19: Move MacroAssembler::negateDouble into generic macro assembler

Comment 24

[Hannes Verschore [:h4writer]]

Comment on attachment 8694043 [details] [diff] [review]
Part 5: Move MacroAssembler::addConstantFloat32 into generic macro assembler

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

Just looked at the source and "addConstantFloat32" is not used in the source anymore.
Instead of moving this, can you please remove this signature.

r+ for removing

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8694057 [details] [diff] [review]
Part 18: Move MacroAssembler::negateFloat into generic macro assembler

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

Cool. negateFloat isn't used on arm64, but I guess this will be in the future.

Comment 26

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8694038 [details] [diff] [review]
Part 1: Move MacroAssembler::addPtr into generic macro assembler

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

::: js/src/jit/MacroAssembler.h
@@ +731,5 @@
> +    inline void addPtr(Register src1, Register src2, Register dest) DEFINED_ON(arm64);
> +    inline void addPtr(Imm32 imm, Register dest) PER_ARCH;
> +    inline void addPtr(Imm32 imm, Register src, Register dest) DEFINED_ON(arm64);
> +    inline void addPtr(ImmWord imm, Register dest) PER_ARCH;
> +    inline void addPtr(ImmPtr imm, Register dest) PER_SHARED_ARCH;

This function seems to always delegate to the ImmWord variant (except on arm64 where it should), remove PER_SHARED_ARCH and move it MacroAssembler-inl.h

@@ +733,5 @@
> +    inline void addPtr(Imm32 imm, Register src, Register dest) DEFINED_ON(arm64);
> +    inline void addPtr(ImmWord imm, Register dest) PER_ARCH;
> +    inline void addPtr(ImmPtr imm, Register dest) PER_SHARED_ARCH;
> +    inline void addPtr(Imm32 imm, const Address& dest) DEFINED_ON(mips_shared, arm, arm64, x86, x64);
> +    inline void addPtr(Imm32 imm, const Operand& dest) DEFINED_ON(x64, x86);

We should avoid "Operand"s on the MacroAssembler, as Operands are placeholders which are architecture specific.
Thus I think having

  addPtr(Imm32 imm, const AbsoluteAddress& dest) DEFINED_ON(x86, x64)

might work too.

I would be fine if you do that as part of a follow-uyp as I know these kind of patches are sometimes painful to reorder/modify within a stack a patches.

::: js/src/jit/arm64/MacroAssembler-arm64-inl.h
@@ +146,5 @@
>  
>  void
> +MacroAssembler::addPtr(Register src, Register dest)
> +{
> +    Add(ARMRegister(dest, 64), ARMRegister(dest, 64), Operand(ARMRegister(src, 64)));

nit: maybe use   addPtr(src, dest, dest);

@@ +158,5 @@
> +
> +void
> +MacroAssembler::addPtr(Imm32 imm, Register dest)
> +{
> +    Add(ARMRegister(dest, 64), ARMRegister(dest, 64), Operand(imm.value));

nit: maybe use   addPtr(imm, dest, dest);

::: js/src/jit/mips32/MacroAssembler-mips32-inl.h
@@ +92,5 @@
> +void
> +MacroAssembler::addPtr(const Address& src, Register dest)
> +{
> +    loadPtr(src, ScratchRegister);
> +    ma_addu(dest, ScratchRegister);

nit: use addPtr() and move this function to mips-shared.

@@ +145,5 @@
> +{
> +    computeScaledAddress(address, dest);
> +    if (address.offset) {
> +        asMasm().addPtr(Imm32(address.offset), dest);
> +    }

style-nit: no curly braces for one lines statements.

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +137,5 @@
> +{
> +    computeScaledAddress(address, dest);
> +    if (address.offset) {
> +        asMasm().addPtr(Imm32(address.offset), dest);
> +    }

style-nit: remove extra curly braces.

::: js/src/jit/x86/MacroAssembler-x86-inl.h
@@ +73,5 @@
> +
> +void
> +MacroAssembler::addPtr(Register src, Register dest)
> +{
> +    add32(src, dest);

nit: maybe use addl instead of add32, such that we remove explicit usage of 32 bits when we want to manipulate an ordinary pointer-size value.  Do the same below.

Comment 27

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8694041 [details] [diff] [review]
Part 3: Move MacroAssembler::add64 into generic macro assembler

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

Thanks for doing this work :)

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8694045 [details] [diff] [review]
Part 7: Move MacroAssembler::addConstantDouble into generic macro assembler

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

Thanks!

::: js/src/jit/x86/MacroAssembler-x86-inl.h
@@ +130,5 @@
> +    Double* dbl = getDouble(d);
> +    if (!dbl)
> +        return;
> +    masm.vaddsd_mr(nullptr, dest.encoding(), dest.encoding());
> +    dbl->uses.append(CodeOffsetLabel(masm.size()));

I think we should call propagateOOM() here but there's similar code in other functions, so let's fix them all at once. I filed bug 1229396.

Comment 29

[Sean Stangl [:sstangl]]

Comment on attachment 8694049 [details] [diff] [review]
Part 11: Move MacroAssembler::mul64 into generic macro assembler

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

::: js/src/jit/MacroAssembler.h
@@ +762,5 @@
>      inline void subDouble(FloatRegister src, FloatRegister dest) PER_SHARED_ARCH;
>  
>      inline void mul32(Register src1, Register src2, Register dest, Label* onOver, Label* onZero) DEFINED_ON(arm64);
>  
> +    inline void mul64(Imm64 imm, Register64 dest) PER_ARCH;

Would it not make sense to leave Register64 as a const reference?

Comment 30

[Lars T Hansen [:lth]]

Comment on attachment 8694046 [details] [diff] [review]
Part 8: Move MacroAssembler::subPtr into generic macro assembler

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

The patch is fine but there's an abuse of APIs here that I think needs to be addressed, either by documenting platform-dependent aspects of the API (flags are set) in the platform implementation so that it can be relied on, or by avoiding the abuse of asMasm() when the flag bits are needed and instead using the platform implementation directly, which is easy on x86.  See specific comment below.

::: js/src/jit/arm/MacroAssembler-arm-inl.h
@@ +312,5 @@
> +void
> +MacroAssemblerARMCompat::decBranchPtr(Condition cond, Register lhs, Imm32 imm, Label* label)
> +{
> +    asMasm().subPtr(imm, lhs);
> +    branch32(cond, lhs, Imm32(0), label);

Seems to me we could do better here by relying on the flag bits being set by the subtract (in that case we'd not use asMasm().subPtr but ma_subs, maybe) but this is just code copied over so OK with me.  File a followup bug to investigate this?

::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
@@ +112,5 @@
> +MacroAssembler::subPtr(Imm32 imm, Register dest)
> +{
> +    ma_dsubu(dest, dest, imm);
> +}
> +

Curious that this is not Register64, cf add64().

::: js/src/jit/x64/MacroAssembler-x64-inl.h
@@ +122,5 @@
> +MacroAssembler::subPtr(Register src, Register dest)
> +{
> +    subq(src, dest);
> +}
> +

Same comment as for x86, below.

::: js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h
@@ +154,5 @@
> +MacroAssemblerX86Shared::decBranchPtr(Condition cond, Register lhs, Imm32 imm, Label* label)
> +{
> +    asMasm().subPtr(imm, lhs);
> +    j(cond, label);
> +}

This requires the flag bits to be set, but setting the flag bits are not part of that API.  The guarantee needs to be stated somewhere.  See following comment.

::: js/src/jit/x86/MacroAssembler-x86-inl.h
@@ +138,5 @@
> +MacroAssembler::subPtr(Register src, Register dest)
> +{
> +    subl(src, dest);
> +}
> +

There is code elsewhere (see patch above) that depends on the x86 realizations of these functions returning with the flags set properly.  That contract is not part of the common MacroAssembler API - it makes no sense on MIPS or PPC - so it better be part of the contract here if code is going to rely on it.  The easiest way is probably to add a comment to each function that says that it must exit with the flags set as for subtract.  (This goes for a ton of these functions, so it could be tedious with all the commenting, but the contract needs to be documented somehow.  IIRC some ARM code plays similar tricks, relying on some operations being implemented with eg sub.s and not just sub.)

Comment 31

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Hannes Verschore [:h4writer] from comment #24)
> Comment on attachment 8694043 [details] [diff] [review]
> Part 5: Move MacroAssembler::addConstantFloat32 into generic macro assembler
> 
> Review of attachment 8694043 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just looked at the source and "addConstantFloat32" is not used in the source
> anymore.
> Instead of moving this, can you please remove this signature.
> 
> r+ for removing

Removed locally.


(In reply to Sean Stangl [:sstangl] from comment #29)
> Comment on attachment 8694049 [details] [diff] [review]
> Part 11: Move MacroAssembler::mul64 into generic macro assembler
> 
> Review of attachment 8694049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MacroAssembler.h
> @@ +762,5 @@
> >      inline void subDouble(FloatRegister src, FloatRegister dest) PER_SHARED_ARCH;
> >  
> >      inline void mul32(Register src1, Register src2, Register dest, Label* onOver, Label* onZero) DEFINED_ON(arm64);
> >  
> > +    inline void mul64(Imm64 imm, Register64 dest) PER_ARCH;
> 
> Would it not make sense to leave Register64 as a const reference?

Changed back to const reference.


(In reply to Lars T Hansen [:lth] from comment #30)
> ::: js/src/jit/arm/MacroAssembler-arm-inl.h
> @@ +312,5 @@
> > +void
> > +MacroAssemblerARMCompat::decBranchPtr(Condition cond, Register lhs, Imm32 imm, Label* label)
> > +{
> > +    asMasm().subPtr(imm, lhs);
> > +    branch32(cond, lhs, Imm32(0), label);
> 
> Seems to me we could do better here by relying on the flag bits being set by
> the subtract (in that case we'd not use asMasm().subPtr but ma_subs, maybe)
> but this is just code copied over so OK with me.  File a followup bug to
> investigate this?

filed bug 1229802.


> ::: js/src/jit/mips64/MacroAssembler-mips64-inl.h
> @@ +112,5 @@
> > +MacroAssembler::subPtr(Imm32 imm, Register dest)
> > +{
> > +    ma_dsubu(dest, dest, imm);
> > +}
> > +
> 
> Curious that this is not Register64, cf add64().

Register64 is used in arch independent 64bit API, to pass 2 registers on 32bit arch.  Pointer-sized operation doesn't need it.


> ::: js/src/jit/x86/MacroAssembler-x86-inl.h
> @@ +138,5 @@
> > +MacroAssembler::subPtr(Register src, Register dest)
> > +{
> > +    subl(src, dest);
> > +}
> > +
> 
> There is code elsewhere (see patch above) that depends on the x86
> realizations of these functions returning with the flags set properly.  That
> contract is not part of the common MacroAssembler API - it makes no sense on
> MIPS or PPC - so it better be part of the contract here if code is going to
> rely on it.  The easiest way is probably to add a comment to each function
> that says that it must exit with the flags set as for subtract.  (This goes
> for a ton of these functions, so it could be tedious with all the
> commenting, but the contract needs to be documented somehow.  IIRC some ARM
> code plays similar tricks, relying on some operations being implemented with
> eg sub.s and not just sub.)

Replaced subPtr calls with subl and subq, and add32 in Part2 with addl also.

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Move MacroAssembler::addPtr into generic macro assembler. r=nbp

Just noticed that fixed patch fails in check_macroassembler_style.py

So, I declared addPtr(ImmPtr, Register) without PER_* or DEFINED* things, and defined impl in MacroAssembler-inl.h (with #ifndef JS_CODEGEN_ARM64) and MacroAssembler-arm64-inl.h.

And the output of check_macroassembler_style.py is following:

  --- check_macroassembler_style.py declared syntax
  +++ check_macroassembler_style.py found definitions
  @@ -13,6 +13,8 @@
       is defined in mips-shared/MacroAssembler-mips-shared.cpp
       is defined in arm/MacroAssembler-arm.cpp
       is defined in arm64/MacroAssembler-arm64.cpp
  +inline #ifndef JS_CODEGEN_ARM64 void addPtr(ImmPtr, Register);
  +    is defined in MacroAssembler-inl.h
   inline ABIFunctionType signature() const;
       is defined in MacroAssembler-inl.h
   inline CodeOffset PushWithPatch(ImmPtr);
  @@ -62,8 +64,8 @@
       is defined in mips-shared/MacroAssembler-mips-shared-inl.h
       is defined in arm/MacroAssembler-arm-inl.h
       is defined in arm64/MacroAssembler-arm64-inl.h
  -inline void addPtr(ImmPtr, Register);
  -    is defined in MacroAssembler-inl.h
  +inline void addPtr(ImmPtr, Register) DEFINED_ON(arm64);
  +    is defined in arm64/MacroAssembler-arm64-inl.h
   inline void addPtr(ImmWord, Register) PER_ARCH;
       is defined in x86/MacroAssembler-x86-inl.h
       is defined in x64/MacroAssembler-x64-inl.h
  TEST-UNEXPECTED-FAIL | check_macroassembler_style.py | actual output does not match expected output;  diff is above


Maybe I need to add another DEFINED* syntax for this case?
or is there any workaround?

Comment 33

[Nicolas B. Pierron [:nbp]]

(In reply to Tooru Fujisawa [:arai] from comment #32)
> Created attachment 8698419 [details] [diff] [review]
> Part 1: Move MacroAssembler::addPtr into generic macro assembler. r=nbp
> 
> Just noticed that fixed patch fails in check_macroassembler_style.py

Can you paste the issue reported by check-masm Makefile target before the modification made with the #ifndef.

> So, I declared addPtr(ImmPtr, Register) without PER_* or DEFINED* things,
> and defined impl in MacroAssembler-inl.h (with #ifndef JS_CODEGEN_ARM64) and
> MacroAssembler-arm64-inl.h.

I am quite happy that this failed, as we should not add any #if in our MacroAssembler header anymore.

Comment 34

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf6dd40d7044810449e09265a6f00b7b41ca021
Bug 1229057 - Part 1: Move MacroAssembler::addPtr into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/400fc382c57b32e2ca4d66cddc286529016bab85
Bug 1229057 - Part 2: Move MacroAssembler::add32 into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/424abf32becba089b76dd2c5c35810b57e525bd1
Bug 1229057 - Part 3: Move MacroAssembler::add64 into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b26d40c14b763180ea4703b223523dd8a3aef8
Bug 1229057 - Part 4: Move MacroAssembler::addFloat32 into generic macro assembler. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/19114c7f1ddb95a3d18c52bcb3f733d494ae17e4
Bug 1229057 - Part 5: Remove unused MacroAssemblerX86::addConstantFloat32. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/f33d197c6c8085baefc0672800ddcfea5caba522
Bug 1229057 - Part 6: Move MacroAssembler::addDouble into generic macro assembler. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/184f8cbcef7e3dc5da124960dcee9f76e88a980f
Bug 1229057 - Part 7: Move MacroAssembler::addConstantDouble into generic macro assembler. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae0723cd69e79ad2d31798c81ac619cf90f03cb9
Bug 1229057 - Part 8: Move MacroAssembler::subPtr into generic macro assembler. r=lth

https://hg.mozilla.org/integration/mozilla-inbound/rev/0427077032e0803ab923c0e08ce600f22c2ab0f0
Bug 1229057 - Part 9: Move MacroAssembler::subDouble into generic macro assembler. r=lth

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d4c7b21f8f85d04063c94d75188b5a263e504b
Bug 1229057 - Part 10: Move MacroAssembler::mul32 into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/417cbfd6c1b6fa736d6a40ae55f61620f8fe5ae9
Bug 1229057 - Part 11: Move MacroAssembler::mul64 into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb58402c22b20f453f1a3784bb5916479d3a3a47
Bug 1229057 - Part 12: Move MacroAssembler::mulBy3 into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/72728bf62ff4754ba4eb2a1967330e17d7b6472a
Bug 1229057 - Part 13: Move MacroAssembler::mulDoublePtr into generic macro assembler. r=djvj

https://hg.mozilla.org/integration/mozilla-inbound/rev/a697223488a3de54b975d905e742177130a5fb1e
Bug 1229057 - Part 14: Move MacroAssembler::mulDouble into generic macro assembler. r=djvj

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a276896d967abe94579451a56c14514284e673e
Bug 1229057 - Part 15: Move MacroAssembler::divDouble into generic macro assembler. r=djvj

https://hg.mozilla.org/integration/mozilla-inbound/rev/e96fe411021280dd9fa25d05d62599fbe023226d
Bug 1229057 - Part 16: Move MacroAssembler::inc64 into generic macro assembler. r=bhackett

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4aeb2be1a139993b35efdf3cf1ee0c6840512d
Bug 1229057 - Part 17: Move MacroAssembler::neg32 into generic macro assembler. r=bhackett

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf62c71e020f136183acb3b4c71e5290c3520004
Bug 1229057 - Part 18: Move MacroAssembler::negateFloat into generic macro assembler. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1ce09c4c1c81aa92e21010e3c1f4fe32957572
Bug 1229057 - Part 19: Move MacroAssembler::negateDouble into generic macro assembler. r=bbouvier

Comment 35

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

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/74c480c100c8 for android build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=18612548&repo=mozilla-inbound

Comment 36

[Tooru Fujisawa [:arai]]

The issue is that addPtr is now defined not in MacroAssembler.h but in MacroAssembler-<arch>-inl.h, but addToStackPtr in MacroAssembler.h calls addPtr.
So, I think I need to move addToStackPtr to MacroAssembler-inl.h, and do same thing for dependent things: move FrameInfo::pop and FrameInfo::popn from BaselineFrameInfo.h to newly created BaselineFrameInfo-inl.h, and include them from some files.

Will post a followup patch for Part 1 shortly, after some more testing.

Comment 37

[Tooru Fujisawa [:arai]]

Attachment: Part 1 followup: Move addToStackPtr and dependent things to -inl.h.

moved addToStackPtr and addStackPtrTo to MacroAssembler-inl.h, and moved FrameInfo::pop and FrameInfo::popn to BaselineFrameInfo-inl.h.

Comment 38

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8699043 [details] [diff] [review]
Part 1 followup: Move addToStackPtr and dependent things to -inl.h.

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

::: js/src/jit/MacroAssembler-inl.h
@@ +341,5 @@
>      branch32(cond, scratch, Imm32(bit), label);
>  }
>  
> +template <typename T> void
> +MacroAssembler::addToStackPtr(T t)

Add a  #ifndef JS_CODEGEN_ARM64  to mirror the header file.

Comment 39

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2145800218a3d07f1b9d22c3358e5240f9dafefb
Bug 1229057 - Part 1: Move MacroAssembler::addPtr into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/27f2890d8b27268e1126dba1536e995bd85baad9
Bug 1229057 - Part 1 followup: Move addToStackPtr and dependent things to -inl.h. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/b75139cf07398def69987534dae7d26af89770c2
Bug 1229057 - Part 2: Move MacroAssembler::add32 into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/275f7590bbe659457b349104a1b984de762dea09
Bug 1229057 - Part 3: Move MacroAssembler::add64 into generic macro assembler. r=nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0947717a9fe5c60781d17c7e580d7ad7adbdb6
Bug 1229057 - Part 4: Move MacroAssembler::addFloat32 into generic macro assembler. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/2491e453b8d049a1e27e6ab608c121d25ea1882b
Bug 1229057 - Part 5: Remove unused MacroAssemblerX86::addConstantFloat32. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/23bd21ccb964f7fafff6688bafb3f1b11215cb15
Bug 1229057 - Part 6: Move MacroAssembler::addDouble into generic macro assembler. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/39dc8792221d9205de8dd10a519e620841d6980c
Bug 1229057 - Part 7: Move MacroAssembler::addConstantDouble into generic macro assembler. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/81fddea678a7bfea7225c96d00c25a149f52889e
Bug 1229057 - Part 8: Move MacroAssembler::subPtr into generic macro assembler. r=lth

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d087c8f5c73979f28167cd39df8795836c39959
Bug 1229057 - Part 9: Move MacroAssembler::subDouble into generic macro assembler. r=lth

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca2e4823afec2aa986c7ee76705fe365399d1a80
Bug 1229057 - Part 10: Move MacroAssembler::mul32 into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/3482ddbcca569e67b8b477c0d6dfe26db8c324c5
Bug 1229057 - Part 11: Move MacroAssembler::mul64 into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/0066457f638ecfb6f2a1b96e91d6d99487dcd7ce
Bug 1229057 - Part 12: Move MacroAssembler::mulBy3 into generic macro assembler. r=sstangl

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2635431b09592d64b709451db402ad053d3878b
Bug 1229057 - Part 13: Move MacroAssembler::mulDoublePtr into generic macro assembler. r=djvj

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4f8ebb128fc15971f044b4f643cc85ab7172c0
Bug 1229057 - Part 14: Move MacroAssembler::mulDouble into generic macro assembler. r=djvj

https://hg.mozilla.org/integration/mozilla-inbound/rev/430ce92b022ca57ff05520310f1c347fcaaf4260
Bug 1229057 - Part 15: Move MacroAssembler::divDouble into generic macro assembler. r=djvj

https://hg.mozilla.org/integration/mozilla-inbound/rev/764452e009e2de62eb9bc8eb6b898d3c45616e14
Bug 1229057 - Part 16: Move MacroAssembler::inc64 into generic macro assembler. r=bhackett

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe6173757f32aff365522cb3d8f4a6f1f7f14fd1
Bug 1229057 - Part 17: Move MacroAssembler::neg32 into generic macro assembler. r=bhackett

https://hg.mozilla.org/integration/mozilla-inbound/rev/7677e7466c249741d51560347285ad13fcd60945
Bug 1229057 - Part 18: Move MacroAssembler::negateFloat into generic macro assembler. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/edc36eaff15ff316dbc6cbcea906aca974f987d9
Bug 1229057 - Part 19: Move MacroAssembler::negateDouble into generic macro assembler. r=bbouvier

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2145800218a3
https://hg.mozilla.org/mozilla-central/rev/27f2890d8b27
https://hg.mozilla.org/mozilla-central/rev/b75139cf0739
https://hg.mozilla.org/mozilla-central/rev/275f7590bbe6
https://hg.mozilla.org/mozilla-central/rev/7f0947717a9f
https://hg.mozilla.org/mozilla-central/rev/2491e453b8d0
https://hg.mozilla.org/mozilla-central/rev/23bd21ccb964
https://hg.mozilla.org/mozilla-central/rev/39dc8792221d
https://hg.mozilla.org/mozilla-central/rev/81fddea678a7
https://hg.mozilla.org/mozilla-central/rev/0d087c8f5c73
https://hg.mozilla.org/mozilla-central/rev/ca2e4823afec
https://hg.mozilla.org/mozilla-central/rev/3482ddbcca56
https://hg.mozilla.org/mozilla-central/rev/0066457f638e
https://hg.mozilla.org/mozilla-central/rev/c2635431b095
https://hg.mozilla.org/mozilla-central/rev/8c4f8ebb128f
https://hg.mozilla.org/mozilla-central/rev/430ce92b022c
https://hg.mozilla.org/mozilla-central/rev/764452e009e2
https://hg.mozilla.org/mozilla-central/rev/fe6173757f32
https://hg.mozilla.org/mozilla-central/rev/7677e7466c24
https://hg.mozilla.org/mozilla-central/rev/edc36eaff15f