Closed Bug 1140954 Opened 9 years ago Closed 9 years ago

MIPS64 port for SpiderMonkey JIT

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: hev, Assigned: hev)

References

(Blocks 1 open bug)

Details

Attachments

(1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305221847

Steps to reproduce:

This bug should track the upstream process for MIPS64 port. Port contains Baseline Compiler, IonMonkey and Odinmonkey.

Current code for MIPS64 port is host at:
https://github.com/heiher/gecko-dev

Code will be offered in 5 phases:
- Skeleton code and Assembler
- Baseline Compiler code
- IonMonkey code
- OdinMonkey code
- MIPS64 simulator

Each phase will be separated in multiple patches. Each phase can be built and
tests can be run.
Hardware: x86_64 → Other
Attached patch Architecture-mips64.patch (obsolete) — Splinter Review
Attachment #8577053 - Flags: review?(nicolas.b.pierron)
Attachment #8577053 - Flags: review?(branislav.rankov)
Assignee: nobody → r
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8577053 [details] [diff] [review]
Architecture-mips64.patch

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

By looking at this, and your code on Github, it seems that you want to copy all Mips files into a new folder. I don't think that this is a good way to go. You are duplicating a lot of code.
I think that you should merge your changes into Mips port. It will be easier to maintain that way.

Otherwise, this patch looks good. I would like to take a better look at it when it is merged.

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

Why do you need encoding() ? It does the same thing as code().
Attachment #8577053 - Flags: review?(branislav.rankov) → review-
(In reply to Branislav Rankov [:rankov] from comment #2)
> ::: js/src/jit/mips64/Architecture-mips64.h
> @@ +393,5 @@
> > +    }
> > +    Encoding encoding() const {
> > +        MOZ_ASSERT(!isInvalid());
> > +        return Code(code_  | (kind_ << 5));
> > +    }
> 
> Why do you need encoding() ? It does the same thing as code().

encoding() and code() will serve 2 different purpose, and this change is made its way recently to x86/x64.  And I expect to rename code()  to  index/registerIndex  to locate the register in the  SetType.
(In reply to Branislav Rankov [:rankov] from comment #2)
> Comment on attachment 8577053 [details] [diff] [review]
> Architecture-mips64.patch
> 
> Review of attachment 8577053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> By looking at this, and your code on Github, it seems that you want to copy
> all Mips files into a new folder. I don't think that this is a good way to
> go. You are duplicating a lot of code.
> I think that you should merge your changes into Mips port. It will be easier
> to maintain that way.

Are you suggesting to have a  mips-shared  directory, and a mips32 & mips64 directory for each specific parts?
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> (In reply to Branislav Rankov [:rankov] from comment #2)
> > Comment on attachment 8577053 [details] [diff] [review]
> > Architecture-mips64.patch
> > 
> > Review of attachment 8577053 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > By looking at this, and your code on Github, it seems that you want to copy
> > all Mips files into a new folder. I don't think that this is a good way to
> > go. You are duplicating a lot of code.
> > I think that you should merge your changes into Mips port. It will be easier
> > to maintain that way.
> 
> Are you suggesting to have a  mips-shared  directory, and a mips32 & mips64
> directory for each specific parts?

I think this is a good idea, like x86 and x64.
(In reply to Heiher from comment #1)
> Created attachment 8577053 [details] [diff] [review]
> Architecture-mips64.patch

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> (In reply to Branislav Rankov [:rankov] from comment #2)
> > Comment on attachment 8577053 [details] [diff] [review]
> > Architecture-mips64.patch
> > 
> > Review of attachment 8577053 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > By looking at this, and your code on Github, it seems that you want to copy
> > all Mips files into a new folder. I don't think that this is a good way to
> > go. You are duplicating a lot of code.
> > I think that you should merge your changes into Mips port. It will be easier
> > to maintain that way.
> 
> Are you suggesting to have a  mips-shared  directory, and a mips32 & mips64
> directory for each specific parts?

(In reply to Branislav Rankov [:rankov] from comment #2)
> Comment on attachment 8577053 [details] [diff] [review]
> Architecture-mips64.patch
> 
> Review of attachment 8577053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> By looking at this, and your code on Github, it seems that you want to copy
> all Mips files into a new folder. I don't think that this is a good way to
> go. You are duplicating a lot of code.
> I think that you should merge your changes into Mips port. It will be easier
> to maintain that way.
> 
> Otherwise, this patch looks good. I would like to take a better look at it
> when it is merged.
> 
> ::: js/src/jit/mips64/Architecture-mips64.h
> @@ +393,5 @@
> > +    }
> > +    Encoding encoding() const {
> > +        MOZ_ASSERT(!isInvalid());
> > +        return Code(code_  | (kind_ << 5));
> > +    }
> 
> Why do you need encoding() ? It does the same thing as code().

I will do that at next week, there have two way maybe to go: 1. Split the shared parts of mips to shared directory, rename mips to mips32. last push mips64. 2. First push mips64, split the shared parts of mips and  mips64 to shared directory, rename mips to mips32. Which one do you think is better?
I think that most of the current code will be common for mips32 and mips64. So, I would keep the code in current folder and only add new files if they are necessary. In my mind, there can be one port that serves both mips64 and mips32.

So, I would propose this plan:
1. Add any changes that are compatible with both mips32 and mips64 to the current code.
2. Add mips64 specific parts of the code (create new files if needed).
3. If needed, move mips32 specific parts to separate files.
4. Enable build for mips64.
(In reply to Branislav Rankov [:rankov] from comment #7)
> I think that most of the current code will be common for mips32 and mips64.
> So, I would keep the code in current folder and only add new files if they
> are necessary. In my mind, there can be one port that serves both mips64 and
> mips32.
> 
> So, I would propose this plan:
> 1. Add any changes that are compatible with both mips32 and mips64 to the
> current code.
> 2. Add mips64 specific parts of the code (create new files if needed).
> 3. If needed, move mips32 specific parts to separate files.
> 4. Enable build for mips64.

I think this is also a solution, but upstream seems don't like too many macro branches in code from I trying to merge mipsn32 into mips. Or we let SpiderMonkey/MIPS64 work first? And then new a bug to talk about how to merge they.
(In reply to Heiher from comment #8)
> I think this is also a solution, but upstream seems don't like too many
> macro branches in code from I trying to merge mipsn32 into mips. Or we let
> SpiderMonkey/MIPS64 work first? And then new a bug to talk about how to
> merge they.

If we first land mips64, and then try to merge it, there will be a lot of unnecessary patches and it will prolong reviewing process.

The too many macro branches comment is from bug 985881 I think. You got a r+ there. I don't think that you need many macro branches in other files. Anyway, if we want to avoid these macro branches, you can copy just the Architecture-mips.h file and merge Architecture-mips.cpp.

Here is the list of files that I think might need partial duplication (similar to X86/X64):
Architecture-mips.h 
MacroAssembler-mips.cpp
MacroAssembler-mips.h
Simulator-mips.cpp
Simulator-mips.h
Trampoline-mips.cpp

Even these can be merged completely with some modifications. But lets work file by file and see what we get in the end.
Comment on attachment 8577053 [details] [diff] [review]
Architecture-mips64.patch

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

::: js/src/jit/mips64/Architecture-mips64.h
@@ +35,5 @@
> +static const uint32_t ShadowStackSpace = 0;
> +
> +// Size of each bailout table entry.
> +// For MIPS this is 2 instructions relative call.
> +static const uint32_t BAILOUT_TABLE_ENTRY_SIZE = 2 * sizeof(uint32_t);

On 64 bits, you probably don't want to use a bailout table at all.

@@ +315,5 @@
> +    static const SetType AllDoubleMask = AllPhysMask * SpreadDouble;
> +
> +    static const SetType NonVolatileMask =
> +      ( (1 << FloatRegisters::f20) |
> +        (1 << FloatRegisters::f22) |

This mask sounds weird, as there is no more overlapping of registers.  Why wouldn't this mask be continuous?
Attachment #8577053 - Flags: review?(nicolas.b.pierron)
(In reply to Branislav Rankov [:rankov] from comment #9)
> (In reply to Heiher from comment #8)
> > I think this is also a solution, but upstream seems don't like too many
> > macro branches in code from I trying to merge mipsn32 into mips. Or we let
> > SpiderMonkey/MIPS64 work first? And then new a bug to talk about how to
> > merge they.
> 
> If we first land mips64, and then try to merge it, there will be a lot of
> unnecessary patches and it will prolong reviewing process.
> 
> The too many macro branches comment is from bug 985881 I think. You got a r+
> there. I don't think that you need many macro branches in other files.
> Anyway, if we want to avoid these macro branches, you can copy just the
> Architecture-mips.h file and merge Architecture-mips.cpp.
> 
> Here is the list of files that I think might need partial duplication
> (similar to X86/X64):
> Architecture-mips.h 
> MacroAssembler-mips.cpp
> MacroAssembler-mips.h
> Simulator-mips.cpp
> Simulator-mips.h
> Trampoline-mips.cpp
> 
> Even these can be merged completely with some modifications. But lets work
> file by file and see what we get in the end.

I will try to merge mips64 into mips directory. Maybe just Architecture-mips, Assembly-mips, Simulator-mips, a part of Macro Assembly-mips and some header files can be shared I think, because mips64 uses punbox64 not compatible with mips32. Thank you.
(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> Comment on attachment 8577053 [details] [diff] [review]
> Architecture-mips64.patch
> 
> Review of attachment 8577053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips64/Architecture-mips64.h
> @@ +35,5 @@
> > +static const uint32_t ShadowStackSpace = 0;
> > +
> > +// Size of each bailout table entry.
> > +// For MIPS this is 2 instructions relative call.
> > +static const uint32_t BAILOUT_TABLE_ENTRY_SIZE = 2 * sizeof(uint32_t);
> 
> On 64 bits, you probably don't want to use a bailout table at all.
> 
> @@ +315,5 @@
> > +    static const SetType AllDoubleMask = AllPhysMask * SpreadDouble;
> > +
> > +    static const SetType NonVolatileMask =
> > +      ( (1 << FloatRegisters::f20) |
> > +        (1 << FloatRegisters::f22) |
> 
> This mask sounds weird, as there is no more overlapping of registers.  Why
> wouldn't this mask be continuous?

Thank you tell me bailout table not needed on 64-bit platform.

My bad, I forgot the floating registers usage of mips64 isn't like mipsn32. I will fix it, thanks.
Attachment #8577053 - Attachment is obsolete: true
I have try to merge mips64 into mips, mips64 has too many difference with mips32, it's hard to do. looks code, mips64 doesn't like mipsn32 that the most of codes can be shared between. this is not to say that is can't to do, that makes the code looks mess. so, my suggestion is seperate similar to mips and mips64 in Google V8. Could you accept it? thanks!
(In reply to Heiher from comment #13)
> I have try to merge mips64 into mips, mips64 has too many difference with
> mips32, it's hard to do. looks code, mips64 doesn't like mipsn32 that the
> most of codes can be shared between. this is not to say that is can't to do,
> that makes the code looks mess. so, my suggestion is seperate similar to
> mips and mips64 in Google V8. Could you accept it? thanks!

To be honest, I think the v8 approach to separate the CodeGenerator for each architecture is not acceptable.

What we prefer instead is to keep as much code shared to avoid the multiplication of issues which could be solved in one location.  Especially since code duplication is the easiest way to make mistakes and security issues.

What we are going to for all Architecture, is to remove all MacroAssembler, by a shared MacroAssembler (started in Bug 1121613).  Also for x64 & x86, the architecture difference are now unified behind constants which enable us to share this code (soon in Bug 1134626).

On the other hand.  #if  statements should be limited, because each time you add one, you have to branch while reading the file.
Ok, I see you point. Actual, could you please tell me what are must be shared between mips and mips64 that is acceptable? how many new files and macro branches are allowed? and must in mips directory?
(In reply to Heiher from comment #15)
> Ok, I see you point. Actual, could you please tell me what are must be
> shared between mips and mips64 that is acceptable? how many new files and
> macro branches are allowed? and must in mips directory?

I do not know much about the two architectures and I have not yet reviewed the code, so you and Branislav might be the best person to call that for the moment.
Ok, I don't know how to merge mips64 into mips looks good now. I can do only let mips64 works. I will waiting for the framework changes and merge them into my repository. someday, we found x86 & x64 merged and no redundant, I will reference it and push again.
All jit-tests with --tbpl are passed on Loongson3 platform now. The codes at https://github.com/heiher/gecko-dev/releases/tag/mips64-v1.0
Depends on: 1194139
Depends on: 1199535
Depends on: 1090957
Depends on: 1205166
Status: NEW → ASSIGNED
Depends on: 1205167
Depends on: 1213740
Depends on: 1213741
Depends on: 1213742
Depends on: 1213743
Depends on: 1213745
Depends on: 1213746
Depends on: 1213747
Depends on: 1213749
Depends on: 1213750
Depends on: 1213751
Depends on: 1213752
Depends on: 1218636
Depends on: 1218637
Depends on: 1218638
Depends on: 1218639
Depends on: 1218640
Depends on: 1218641
Depends on: 1218642
Blocks: 1218652
No longer blocks: 1218652
Depends on: 1218652
No longer depends on: 1218652
Blocks: 1205524
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.