Move MacroAssembler arithmetic functions to the generic macro assembler.

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks: 1 bug)

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed)

Details

Attachments

(21 attachments)

43.66 KB, patch
nbp
: review+
Details | Diff | Splinter Review
22.16 KB, patch
nbp
: review+
Details | Diff | Splinter Review
12.82 KB, patch
nbp
: review+
Details | Diff | Splinter Review
3.00 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
5.42 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
15.67 KB, patch
jandem
: review+
Details | Diff | Splinter Review
7.58 KB, patch
jandem
: review+
Details | Diff | Splinter Review
36.56 KB, patch
lth
: review+
Details | Diff | Splinter Review
13.09 KB, patch
lth
: review+
Details | Diff | Splinter Review
3.67 KB, patch
sstangl
: review+
Details | Diff | Splinter Review
20.23 KB, patch
sstangl
: review+
Details | Diff | Splinter Review
12.54 KB, patch
sstangl
: review+
Details | Diff | Splinter Review
12.67 KB, patch
djvj
: review+
Details | Diff | Splinter Review
15.32 KB, patch
djvj
: review+
Details | Diff | Splinter Review
12.92 KB, patch
djvj
: review+
Details | Diff | Splinter Review
18.19 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
9.43 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
4.88 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
13.15 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
44.38 KB, patch
Details | Diff | Splinter Review
6.90 KB, patch
nbp
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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

3 years ago
sub32 was moved last week (to support bug 1108290).
(Assignee)

Comment 2

3 years ago
oh, right
thanks :)
(Assignee)

Comment 3

3 years ago
looks like following are also arithmetic functions:
  * neg32
  * negateFloat
  * negateDouble
(Assignee)

Comment 4

3 years ago
Created attachment 8694038 [details] [diff] [review]
Part 1: Move MacroAssembler::addPtr into generic macro assembler
(Assignee)

Comment 5

3 years ago
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
Attachment #8694038 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 6

3 years ago
Created attachment 8694040 [details] [diff] [review]
Part 2: Move MacroAssembler::add32 into generic macro assembler
Attachment #8694040 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 7

3 years ago
Created attachment 8694041 [details] [diff] [review]
Part 3: Move MacroAssembler::add64 into generic macro assembler
Attachment #8694041 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 8

3 years ago
Created attachment 8694042 [details] [diff] [review]
Part 4: Move MacroAssembler::addFloat32 into generic macro assembler
Attachment #8694042 - Flags: review?(hv1989)
(Assignee)

Comment 9

3 years ago
Created attachment 8694043 [details] [diff] [review]
Part 5: Move MacroAssembler::addConstantFloat32 into generic macro assembler
Attachment #8694043 - Flags: review?(hv1989)
(Assignee)

Comment 10

3 years ago
Created attachment 8694044 [details] [diff] [review]
Part 6: Move MacroAssembler::addDouble into generic macro assembler
Attachment #8694044 - Flags: review?(jdemooij)
(Assignee)

Comment 11

3 years ago
Created attachment 8694045 [details] [diff] [review]
Part 7: Move MacroAssembler::addConstantDouble into generic macro assembler
Attachment #8694045 - Flags: review?(jdemooij)
(Assignee)

Comment 12

3 years ago
Created attachment 8694046 [details] [diff] [review]
Part 8: Move MacroAssembler::subPtr into generic macro assembler
Attachment #8694046 - Flags: review?(lhansen)
(Assignee)

Comment 13

3 years ago
Created attachment 8694047 [details] [diff] [review]
Part 9: Move MacroAssembler::subDouble into generic macro assembler
Attachment #8694047 - Flags: review?(lhansen)
(Assignee)

Comment 14

3 years ago
Created attachment 8694048 [details] [diff] [review]
Part 10: Move MacroAssembler::mul32 into generic macro assembler
Attachment #8694048 - Flags: review?(sstangl)
(Assignee)

Comment 15

3 years ago
Created attachment 8694049 [details] [diff] [review]
Part 11: Move MacroAssembler::mul64 into generic macro assembler
Attachment #8694049 - Flags: review?(sstangl)
(Assignee)

Comment 16

3 years ago
Created attachment 8694050 [details] [diff] [review]
Part 12: Move MacroAssembler::mulBy3 into generic macro assembler
Attachment #8694050 - Flags: review?(sstangl)
(Assignee)

Comment 17

3 years ago
Created attachment 8694051 [details] [diff] [review]
Part 13: Move MacroAssembler::mulDoublePtr into generic macro assembler
Attachment #8694051 - Flags: review?(kvijayan)
(Assignee)

Comment 18

3 years ago
Created attachment 8694052 [details] [diff] [review]
Part 14: Move MacroAssembler::mulDouble into generic macro assembler
Attachment #8694052 - Flags: review?(kvijayan)
(Assignee)

Comment 19

3 years ago
Created attachment 8694053 [details] [diff] [review]
Part 15: Move MacroAssembler::divDouble into generic macro assembler
Attachment #8694053 - Flags: review?(kvijayan)
(Assignee)

Comment 20

3 years ago
Created attachment 8694055 [details] [diff] [review]
Part 16: Move MacroAssembler::inc64 into generic macro assembler
Attachment #8694055 - Flags: review?(bhackett1024)
(Assignee)

Comment 21

3 years ago
Created attachment 8694056 [details] [diff] [review]
Part 17: Move MacroAssembler::neg32 into generic macro assembler
Attachment #8694056 - Flags: review?(bhackett1024)
(Assignee)

Comment 22

3 years ago
Created attachment 8694057 [details] [diff] [review]
Part 18: Move MacroAssembler::negateFloat into generic macro assembler
Attachment #8694057 - Flags: review?(benj)
(Assignee)

Comment 23

3 years ago
Created attachment 8694058 [details] [diff] [review]
Part 19: Move MacroAssembler::negateDouble into generic macro assembler
Attachment #8694058 - Flags: review?(benj)
Attachment #8694042 - Flags: review?(hv1989) → review+
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
Attachment #8694043 - Flags: review?(hv1989) → review+
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.
Attachment #8694057 - Flags: review?(benj) → review+
Attachment #8694058 - Flags: review?(benj) → review+
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.
Attachment #8694038 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8694040 - Flags: review?(nicolas.b.pierron) → review+
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 :)
Attachment #8694041 - Flags: review?(nicolas.b.pierron) → review+

Updated

3 years ago
Attachment #8694044 - Flags: review?(jdemooij) → review+
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.
Attachment #8694045 - Flags: review?(jdemooij) → review+

Updated

3 years ago
Attachment #8694048 - Flags: review?(sstangl) → review+
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?
Attachment #8694049 - Flags: review?(sstangl) → review+

Updated

3 years ago
Attachment #8694050 - Flags: review?(sstangl) → review+

Updated

3 years ago
Attachment #8694047 - Flags: review?(lhansen) → review+
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.)
Attachment #8694046 - Flags: review?(lhansen) → review+
Attachment #8694055 - Flags: review?(bhackett1024) → review+
Attachment #8694056 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 31

3 years ago
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.
See Also: → bug 1229802

Updated

3 years ago
Attachment #8694051 - Flags: review?(kvijayan) → review+

Updated

3 years ago
Attachment #8694052 - Flags: review?(kvijayan) → review+

Updated

3 years ago
Attachment #8694053 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 32

3 years ago
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

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?
Flags: needinfo?(nicolas.b.pierron)
(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.
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 34

3 years ago
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
(Assignee)

Comment 36

3 years ago
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.
(Assignee)

Comment 37

3 years ago
Created attachment 8699043 [details] [diff] [review]
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.
Flags: needinfo?(arai.unmht)
Attachment #8699043 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8699043 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 39

3 years ago
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
You need to log in before you can comment on or make changes to this bug.