1194139 - IonMonkey: MIPS32: Split shareable code from mips32 to mips-shared

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

Split files in 3-5 steps, do that one by one, may be easy review:
  1. Architecture-mips32.{h,cpp}
  2. Assember-mips32.{h,cpp}
  ...

Comment 1

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8647414 [details] [diff] [review]
0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch

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

This sounds fine to me, but I think Rankov might want to have a look at this patch too.

::: js/src/jit/mips-shared/Architecture-mips.cpp
@@ +44,5 @@
> +    isSet = true;
> +    return flags;
> +#endif
> +
> +    return false;

nit: uint32_t ?

Comment 3

[Branislav Rankov [:rankov]]

Comment on attachment 8647414 [details] [diff] [review]
0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch

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

I see some places where #ifdef's could be avoided if you leave the code in the mips32 file.

I think that it would be easier to review if we can see how it will look like when mips64 is added. Can you attach the patch that will add mips64 version of these files?

::: js/src/jit/mips-shared/Architecture-mips.h
@@ +75,5 @@
> +        a3 = r7,
> +        t0 = r8,
> +        t1 = r9,
> +        t2 = r10,
> +        t3 = r11,

Are you planing an #ifdef here for MIPS64? You will have a4 - a7 here.

@@ +109,5 @@
> +
> +    static const char* GetName(Code code) {
> +        MOZ_ASSERT(code < Total);
> +        static const char * const Names[] = { "zero", "at", "v0", "v1", "a0", "a1", "a2", "a3",
> +                                              "t0",   "t1", "t2", "t3", "t4", "t5", "t6", "t7",

Seems like an future #idef also.

@@ +128,5 @@
> +    static const uint32_t Allocatable = 14;
> +
> +    typedef uint32_t SetType;
> +    static const SetType AllMask = 0xffffffff;
> +    static const SetType ArgRegMask = (1 << a0) | (1 << a1) | (1 << a2) | (1 << a3);

Future #ifdef?

@@ +137,5 @@
> +        (1 << Registers::a0) |
> +        (1 << Registers::a1) |
> +        (1 << Registers::a2) |
> +        (1 << Registers::a3) |
> +        (1 << Registers::t0) |

Future #ifdef?

@@ +162,5 @@
> +
> +    static const SetType WrapperMask =
> +        VolatileMask |         // = arguments
> +        (1 << Registers::t0) | // = outReg
> +        (1 << Registers::t1);  // = argBase

Another potential #ifdef.

@@ +283,5 @@
> +
> +    static const uint32_t Total = 64;
> +    static const uint32_t TotalDouble = 16;
> +    static const uint32_t TotalSingle = 32;
> +    static const uint32_t Allocatable = 42;

This is also different for mips64.

@@ +300,5 @@
> +         (1ULL << FloatRegisters::f28) |
> +         (1ULL << FloatRegisters::f30)) << 32;
> +
> +    // f20-single and f21-single alias f20-double ...
> +    static const SetType NonVolatileMask =

Another difference with mips64.

Comment 4

[hev [:hev]]

(In reply to Branislav Rankov [:rankov] from comment #3)
> Comment on attachment 8647414 [details] [diff] [review]
> 0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch
> 
> Review of attachment 8647414 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see some places where #ifdef's could be avoided if you leave the code in
> the mips32 file.
> 
> I think that it would be easier to review if we can see how it will look
> like when mips64 is added. Can you attach the patch that will add mips64
> version of these files?
> 
> ::: js/src/jit/mips-shared/Architecture-mips.h
> @@ +75,5 @@
> > +        a3 = r7,
> > +        t0 = r8,
> > +        t1 = r9,
> > +        t2 = r10,
> > +        t3 = r11,
> 
> Are you planing an #ifdef here for MIPS64? You will have a4 - a7 here.
> 
> @@ +109,5 @@
> > +
> > +    static const char* GetName(Code code) {
> > +        MOZ_ASSERT(code < Total);
> > +        static const char * const Names[] = { "zero", "at", "v0", "v1", "a0", "a1", "a2", "a3",
> > +                                              "t0",   "t1", "t2", "t3", "t4", "t5", "t6", "t7",
> 
> Seems like an future #idef also.
> 
> @@ +128,5 @@
> > +    static const uint32_t Allocatable = 14;
> > +
> > +    typedef uint32_t SetType;
> > +    static const SetType AllMask = 0xffffffff;
> > +    static const SetType ArgRegMask = (1 << a0) | (1 << a1) | (1 << a2) | (1 << a3);
> 
> Future #ifdef?
> 
> @@ +137,5 @@
> > +        (1 << Registers::a0) |
> > +        (1 << Registers::a1) |
> > +        (1 << Registers::a2) |
> > +        (1 << Registers::a3) |
> > +        (1 << Registers::t0) |
> 
> Future #ifdef?
> 
> @@ +162,5 @@
> > +
> > +    static const SetType WrapperMask =
> > +        VolatileMask |         // = arguments
> > +        (1 << Registers::t0) | // = outReg
> > +        (1 << Registers::t1);  // = argBase
> 
> Another potential #ifdef.
> 
> @@ +283,5 @@
> > +
> > +    static const uint32_t Total = 64;
> > +    static const uint32_t TotalDouble = 16;
> > +    static const uint32_t TotalSingle = 32;
> > +    static const uint32_t Allocatable = 42;
> 
> This is also different for mips64.
> 
> @@ +300,5 @@
> > +         (1ULL << FloatRegisters::f28) |
> > +         (1ULL << FloatRegisters::f30)) << 32;
> > +
> > +    // f20-single and f21-single alias f20-double ...
> > +    static const SetType NonVolatileMask =
> 
> Another difference with mips64.

Thanks for your advice. ;)

Comment 5

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch

Comment 6

[hev [:hev]]

Attachment: Architecture-mips64.patch

This is a temporary mips64 implement for Architecture. hopes help for review.

Comment 7

[hev [:hev]]

Attachment: 0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch

For refactor floatregisters updated.

Comment 8

[hev [:hev]]

Attachment: Architecture-mips64.patch

This is a temporary mips64 implement for Architecture. hopes help for review.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8648484 [details] [diff] [review]
0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch

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

I think it would be easier to have 2 versions before refactoring the 2.
Implementing a shared version first might be appealing, but I think it make more sense to optimize both before trying to refactor the common parts.

What do you think?

::: js/src/jit/mips-shared/Architecture-mips.h
@@ +299,5 @@
> +    static const SetType SpreadSingle = SetType(1) << (uint32_t(Single) * TotalPhys);
> +    static const SetType SpreadDouble = SetType(1) << (uint32_t(Double) * TotalPhys);
> +    static const SetType SpreadScalar = SpreadSingle | SpreadDouble;
> +    static const SetType SpreadVector = 0;
> +    static const SetType Spread = SpreadScalar | SpreadVector;

As commented on Bug 1194588, this should not be used on mips32.

@@ +361,5 @@
> +    bool isFloat32x4() const { return false; }
> +
> +    Code code() const {
> +        MOZ_ASSERT(!isInvalid());
> +        return Code(reg_  | (kind_ << 5));

Assert that the return value is no larger than the total number of registers.

::: js/src/jit/mips32/Architecture-mips32.h
@@ +103,1 @@
>      static FloatRegister FromCode(uint32_t i) {

Assert that the code is no larger than the total number of registers.

Comment 10

[hev [:hev]]

Attachment: Part 1: Architecture-mips

Please review. thanks ;)

Comment 11

[hev [:hev]]

Attachment: Architecture-mips64.patch

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8650320 [details] [diff] [review]
Part 1: Architecture-mips

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

Thanks, this is easier to review, but I do expect a follow-up patch to pack the register code properly on mips32.

::: js/src/jit/mips-shared/Architecture-mips.cpp
@@ +3,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "jit/mips-shared/Architecture-mips.h"

This file and the corresponding header should be named mips-shared.
This is not a big deal for this file at the moment, but this would be expected for the MacroAssembler-mips-shared.cpp file.

::: js/src/jit/mips-shared/Architecture-mips.h
@@ +239,5 @@
> +        f30,
> +        f31,
> +        invalid_freg
> +    };
> +    typedef FPRegisterID Code;

in a follow-up patch, you should change this to an integer type.

@@ +248,5 @@
> +        double d;
> +    };
> +
> +    static const char* GetName(Code code) {
> +        static const char * const Names[] = { "f0", "f1", "f2", "f3",  "f4", "f5",  "f6", "f7",

In a follow-up patch; As the array is not typed, you should use the Encoding instead of the code.

::: js/src/moz.build
@@ +462,2 @@
>      ]
> +    if CONFIG['JS_CODEGEN_MIPS32']:

Update the previous condition as well.

Comment 13

[hev [:hev]]

Attachment: Part 1.1: Rename Architecture-mips -> Architecture-mips-shared

This patch just only rename Architecture-mips to Architrecture-mips-shared.

Comment 14

[hev [:hev]]

Attachment: 0003-IonMonkey-MIPS-Redefine-FloatRegisters-Code-and-use-.patch

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8651808 [details] [diff] [review]
Part 1.1: Rename Architecture-mips -> Architecture-mips-shared

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

Fold this patch with the previous one.

Comment 16

[hev [:hev]]

Attachment: 0003-IonMonkey-MIPS-Redefine-FloatRegisters-Code-and-use-.patch

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8651823 [details] [diff] [review]
0003-IonMonkey-MIPS-Redefine-FloatRegisters-Code-and-use-.patch

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

::: js/src/jit/mips32/Architecture-mips32.h
@@ +182,4 @@
>      }
>      Encoding encoding() const {
>          MOZ_ASSERT(!isInvalid());
> +        return Encoding(code_ | (kind_ << 5));

Why do you need the kind_?
The encoding is a type-less register, it should only contain a value listed as part of the enum.

Comment 18

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #17)
> Comment on attachment 8651823 [details] [diff] [review]
> 0003-IonMonkey-MIPS-Redefine-FloatRegisters-Code-and-use-.patch
> 
> Review of attachment 8651823 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips32/Architecture-mips32.h
> @@ +182,4 @@
> >      }
> >      Encoding encoding() const {
> >          MOZ_ASSERT(!isInvalid());
> > +        return Encoding(code_ | (kind_ << 5));
> 
> Why do you need the kind_?
> The encoding is a type-less register, it should only contain a value listed
> as part of the enum.

I'm surprised at this, I think just only the code contain type, but I'm not sure. I looked this part in arm and it same as mips32, so I don't modify it. ;)

Comment 19

[hev [:hev]]

Attachment: Part 1.2: Redefine FloatRegisters::Code

Comment 20

[Nicolas B. Pierron [:nbp]]

(In reply to Heiher from comment #18)
> (In reply to Nicolas B. Pierron [:nbp] from comment #17)
> > Comment on attachment 8651823 [details] [diff] [review]
> > 0003-IonMonkey-MIPS-Redefine-FloatRegisters-Code-and-use-.patch
> > 
> > Review of attachment 8651823 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/jit/mips32/Architecture-mips32.h
> > @@ +182,4 @@
> > >      }
> > >      Encoding encoding() const {
> > >          MOZ_ASSERT(!isInvalid());
> > > +        return Encoding(code_ | (kind_ << 5));
> > 
> > Why do you need the kind_?
> > The encoding is a type-less register, it should only contain a value listed
> > as part of the enum.
> 
> I'm surprised at this, I think just only the code contain type, but I'm not
> sure. I looked this part in arm and it same as mips32, so I don't modify it.
> ;)

Ok, then this is a bug in arm.

Comment 21

[hev [:hev]]

Attachment: Part 2: Assembler-mips

Comment 22

[hev [:hev]]

Attachment: Part 2: Assembler-mips

Update for easier review.

Comment 23

[hev [:hev]]

Attachment: Assembler-mips64.patch

Comment 24

[hev [:hev]]

Attachment: Part 3: AtomicOperations-mips

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdaacec0f5a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b900efecde6

Comment 26

[Pulsebot]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fdaacec0f5a1
https://hg.mozilla.org/mozilla-central/rev/3b900efecde6
https://hg.mozilla.org/mozilla-central/rev/b368fb1c13a5

Comment 28

[hev [:hev]]

Attachment: Part 2: Assembler-mips

Update with Bug 986680 changes. otherwise, can't apply it into master.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ecba91f42b5

Comment 30

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6782a855c243 seems one of this changes caused a bustage like:

https://treeherder.mozilla.org/logviewer.html#?job_id=13496016&repo=mozilla-inbound

Comment 31

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d26fdc33c95e

Comment 32

[hev [:hev]]

(In reply to Carsten Book [:Tomcat] from comment #30)
> sorry had to back this out in
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=6782a855c243 seems one of this changes caused a bustage
> like:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=13496016&repo=mozilla-
> inbound

Sorry, it's my mistake, It caused by Bug 1198628. I will trying build with -Werror=sign-cmopare and fix that.

Comment 33

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8654675 [details] [diff] [review]
Part 2: Assembler-mips

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

::: js/src/jit/mips-shared/Assembler-mips-shared.h
@@ +109,5 @@
> +static MOZ_CONSTEXPR_VAR FloatRegister ScratchFloat32Reg = SCRATCH_FLOAT32_REG;
> +static MOZ_CONSTEXPR_VAR FloatRegister ScratchDoubleReg = SCRATCH_DOUBLE_REG;
> +static MOZ_CONSTEXPR_VAR FloatRegister ScratchSimdReg = InvalidFloatReg;
> +static MOZ_CONSTEXPR_VAR FloatRegister SecondScratchFloat32Reg = SECOND_SCRATCH_FLOAT32_REG;
> +static MOZ_CONSTEXPR_VAR FloatRegister SecondScratchDoubleReg = SECOND_SCRATCH_DOUBLE_REG;

If this is architecture specific, do not create a macro to alias it in the generic file, but just move the line to the architecture specific file.
There is no value in having such macro, worse this might let other developers think that they can rely on such macro, which is not intended.

@@ +626,5 @@
> +        }
> +    }
> +};
> +
> +class BaseAssembler : public AssemblerShared

nit: s/BaseAssembler/AssemblerMIPSShared/g

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a560ab743f8

Comment 35

[hev [:hev]]

Please review https://github.com/heiher/gecko-dev/commit/mips32-shared-r1

Thank you!

Comment 36

[Nicolas B. Pierron [:nbp]]

(In reply to Heiher [:hev] from comment #35)
> Please review https://github.com/heiher/gecko-dev/commit/mips32-shared-r1

Can you still attach a patch, in addition to the link, such that I can set a review flag, and that sheriff can cherry-pick your patches.

Also, you might want to request commit access, for pushing to mozilla-inbound your-self.

Comment 37

[hev [:hev]]

Attachment: Part 2: Assembler-mips

https://github.com/heiher/gecko-dev/commit/mips32-shared-r1

thanks.

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a560ab743f8

Comment 39

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8658120 [details] [diff] [review]
Part 2: Assembler-mips

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

Apart from the following question, this patch looks good.

::: js/src/jit/mips32/Assembler-mips32.cpp
@@ +138,4 @@
>  }
>  
>  void
> +AssemblerMIPSShared::executableCopy(uint8_t* buffer)

Why do we have any AssemblerMIPSShared in the mips32.cpp file?

Comment 40

[hev [:hev]]

Comment on attachment 8658120 [details] [diff] [review]
Part 2: Assembler-mips

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

::: js/src/jit/mips32/Assembler-mips32.cpp
@@ +138,4 @@
>  }
>  
>  void
> +AssemblerMIPSShared::executableCopy(uint8_t* buffer)

Because executableCopy for mips32 and mips64 are same prototype, but implementation are different. so declare it in AssemblerMIPSShared and implement in architecture file.

Comment 41

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8658120 [details] [diff] [review]
Part 2: Assembler-mips

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

::: js/src/jit/mips32/Assembler-mips32.cpp
@@ +138,4 @@
>  }
>  
>  void
> +AssemblerMIPSShared::executableCopy(uint8_t* buffer)

I think this would be less confusing if you keep the declaration of this function in the Assembler of mips32 and mips64, and not in mips-shared.

Also do the same for any AssemblerMIPSShared function which are present in this file, otherwise this would be confusing when you would be looking for these functions.  The MacroAssembler is kind of the opposite exception as we are supposed to have only one interface, and this is the reasons why I am adding the macro-annotations to locate the definitions.

Comment 42

[hev [:hev]]

Attachment: Part 2: Assembler-mips v2

Add review opinion.

Comment 43

[hev [:hev]]

Attachment: Assembler-mips-diff-with-priv.patch

Comment 44

[hev [:hev]]

Attachment: Part 4: LIR-mips

https://github.com/heiher/gecko-dev/commit/mips32-shared-r2

Comment 45

[hev [:hev]]

Attachment: Part 5: Lowering-mips

https://github.com/heiher/gecko-dev/commit/mips32-shared-r3

Comment 46

[hev [:hev]]

Attachment: Part 6: BaselineIC-mips

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c20e26ca644

Comment 48

[hev [:hev]]

Attachment: Part 7: Bailouts-mips

https://github.com/heiher/gecko-dev/commit/mips32-shared-r4

Comment 49

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13938933&repo=mozilla-inbound

Comment 50

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2af5bf6ef42a

Comment 51

[hev [:hev]]

Attachment: Part 2: Assembler-mips v3

Fix check_spidermonkey_style.py failure.

Comment 52

[hev [:hev]]

Attachment: Part 5: Lowering-mips

Update v2, Fix check_spidermonkey_style.py failure.

Comment 53

[hev [:hev]]

Attachment: Part 7: Bailouts-mips

Update v2, Fix check_spidermonkey_style.py failure.

Comment 54

[hev [:hev]]

Attachment: Part 2.1: Fix build failure caused by extract MoveEmitter-mips32

I have do build test at every step, I'm sure the builds are passed from part 2 to 7.

When I extract MoveEmitter-mips32, and build failed in Assembler-mips32, some symbols are missing in Assembler-mips32.cpp and declare in Assembler-mips-shared.cpp. In face, I think these classes are in same unifeid cpp source file, so build ok before this.

Comment 55

[hev [:hev]]

Attachment: Part 8: MoveEmitter-mips

https://github.com/heiher/gecko-dev/commit/mips32-shared-r5

Comment 56

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8659805 [details] [diff] [review]
Part 2.1: Fix build failure caused by extract MoveEmitter-mips32

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

::: js/src/jit/mips-shared/Assembler-mips-shared.h
@@ +1247,5 @@
> +
> +    bool read() {
> +        if (!reader_.more())
> +            return false;
> +        offset_ = reader_.readUnsigned();

Couldn't we inline this code in the TraceJumpRelocations function?

Comment 57

[Nicolas B. Pierron [:nbp]]

I think this leave-open madness should end after the part 2.1.
Make a meta-bug, rename this one, and make blockers of the meta bug to handle each step.

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d016a97a99d

Comment 59

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1d016a97a99d