Closed Bug 994716 Opened 6 years ago Closed 5 years ago

IonMonkey MIPS: Bring MIPS code up to speed with the rest of the code

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: rankov, Assigned: rankov)

References

Details

Attachments

(10 files, 14 obsolete files)

14.54 KB, patch
rankov
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
4.34 KB, patch
rankov
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
6.92 KB, patch
jandem
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
27.13 KB, patch
rankov
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
4.62 KB, patch
rankov
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.20 KB, patch
rankov
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
7.85 KB, patch
rankov
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.01 KB, patch
nbp
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.60 KB, patch
nbp
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.28 KB, patch
rankov
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
During upload process in the bug 969375 there have been independent changes in the rest of the code that require changes in the MIPS part. This bug will introduce these changes to MIPS port.
Assignee: nobody → branislav.rankov
Status: NEW → ASSIGNED
Depends on: 969375
Attached patch Style-fixes.patch (obsolete) — Splinter Review
Attachment #8404761 - Flags: review?(jdemooij)
Attachment #8404761 - Flags: review?(jdemooij) → review+
Carry review from previous patch.

I added r=jandem and renamed patch to have the correct order since there will be more patches in the bug.
Attachment #8404761 - Attachment is obsolete: true
Attachment #8406787 - Flags: review+
Attached patch 02-Style-fixes.patch (obsolete) — Splinter Review
Attachment #8405312 - Attachment is obsolete: true
Attached patch 03-TestOverflow.patch (obsolete) — Splinter Review
Attached patch 04-AutoFlushCache.patch (obsolete) — Splinter Review
Attached patch 05-General-updates.patch (obsolete) — Splinter Review
Attachment #8407492 - Flags: review?(nicolas.b.pierron)
Attachment #8407493 - Flags: review?(nicolas.b.pierron)
Attachment #8407494 - Flags: review?(nicolas.b.pierron)
Attachment #8407495 - Flags: review?(nicolas.b.pierron)
Attachment #8407492 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8407493 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8407494 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8407495 [details] [diff] [review]
05-General-updates.patch

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

Remove the 2 parts on which I commented and r=me.

::: js/src/jit/mips/MacroAssembler-mips.cpp
@@ +1481,5 @@
>  
>  bool
>  MacroAssemblerMIPSCompat::buildOOLFakeExitFrame(void *fakeReturnAddr)
>  {
> +    mozilla::DebugOnly<uint32_t> initialDepth = framePushed();

initialDepth is not used in any assertion in this function, it should be removed then.

::: js/src/jit/mips/MacroAssembler-mips.h
@@ +68,5 @@
> +    }
> +    Operand ToType(Operand base);
> +    Address ToType(Address base) {
> +        return ToType(Operand(base)).toAddress();
> +    }

These function sounds nice, but I feel that this should have been added in another patch.
Attachment #8407495 - Flags: review?(nicolas.b.pierron) → review+
Attached patch 05-General-updates.patch (obsolete) — Splinter Review
Carry review from previous patch.

Removed initialDepth definition.

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> These function sounds nice, but I feel that this should have been added in
> another patch.

These changes were introduced to ARM and X86 after MacroAssembler-mips has landed. Part of this change is already in some of the files because not all files were uploaded at the same time.
Attachment #8407495 - Attachment is obsolete: true
Attachment #8408260 - Flags: review+
Carry review from previous patch.

Parts of this patch were moved to patch Trampoline-mips.patch in bug 969375.
Attachment #8408260 - Attachment is obsolete: true
Attachment #8411726 - Flags: review+
Attached patch 06-ScaledAddress.patch (obsolete) — Splinter Review
This patch came from Heiher, so I'm upstreming it with his name.
Attached patch 07-General-updates-2.patch (obsolete) — Splinter Review
Attachment #8411884 - Flags: review?(jdemooij)
Attachment #8411842 - Flags: review?(jdemooij)
A small add to the patch.
Attachment #8411884 - Attachment is obsolete: true
Attachment #8411884 - Flags: review?(jdemooij)
Attachment #8411891 - Flags: review?(jdemooij)
Attachment #8411842 - Flags: review?(jdemooij) → review+
Attachment #8411891 - Flags: review?(jdemooij) → review+
Carry review from previous patch.
Updated r=nbp.
Attachment #8412660 - Flags: review+
Carry review from previous patch.
Updated r=nbp.
Attachment #8407492 - Attachment is obsolete: true
Attachment #8407493 - Attachment is obsolete: true
Attachment #8412662 - Flags: review+
Attached patch 04-AutoFlushCache.patch (obsolete) — Splinter Review
Carry review from previous patch.
Updated r=nbp.
Attachment #8407494 - Attachment is obsolete: true
Attachment #8412663 - Flags: review+
Carry review from previous patch.
Updated r=jandem.
Attachment #8411842 - Attachment is obsolete: true
Comment on attachment 8412664 [details] [diff] [review]
06-ScaledAddress.patch

Carry review from previous patch.
Attachment #8412664 - Flags: review+
Comment on attachment 8412663 [details] [diff] [review]
04-AutoFlushCache.patch

Douglas,

Does this patch affect the work you are doing for cache flush on ARM?
Attachment #8412663 - Flags: feedback?(dtc-moz)
Comment on attachment 8412663 [details] [diff] [review]
04-AutoFlushCache.patch

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

It is proposed to rewrite this code, and the patched code has been removed. Emailed along the patch.
Attachment #8412663 - Flags: feedback?(dtc-moz)
Attached patch 08-General-updates-3.patch (obsolete) — Splinter Review
Removed 04-AutoFlushCache.patch because of comment #20 from Douglas.
Attachment #8412663 - Attachment is obsolete: true
Attachment #8418064 - Flags: review?(nicolas.b.pierron)
Attached patch 09-test-magic-value.patch (obsolete) — Splinter Review
Attachment #8418066 - Flags: review?(nicolas.b.pierron)
Attachment #8406787 - Flags: checkin?(nicolas.b.pierron)
Attachment #8411726 - Flags: checkin?(nicolas.b.pierron)
Attachment #8411891 - Flags: checkin?(nicolas.b.pierron)
Attachment #8412660 - Flags: checkin?(nicolas.b.pierron)
Attachment #8412662 - Flags: checkin?(nicolas.b.pierron)
Attachment #8412664 - Flags: checkin?(nicolas.b.pierron)
Keywords: leave-open
Hi, could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Comment on attachment 8418064 [details] [diff] [review]
08-General-updates-3.patch

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

Remove executableCopy, and r=me

::: js/src/jit/shared/IonAssemblerBuffer.h
@@ +251,5 @@
>          tail->setNext(tmp);
>          tail = tmp;
>      }
>  
> +    void executableCopy(uint8_t *dest_) {

Doesn't this belong in a different patch?
Attachment #8418064 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8418066 [details] [diff] [review]
09-test-magic-value.patch

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

hum … I would prefer if we could open a dedicated bug for this one, as it seems that we have the same issue on ARM and x64 too.
Can you move this patch to the new bug and carry the review to this new bug, thanks.

By the way, the x64 version should just do a 64bits compare in all cases.

::: js/src/jit/mips/MacroAssembler-mips.h
@@ +708,5 @@
>      void branchTestMagic(Condition cond, const BaseIndex &src, Label *label);
>  
>      void branchTestMagicValue(Condition cond, const ValueOperand &val, JSWhyMagic why,
>                                Label *label) {
> +        JS_ASSERT(cond == Equal || cond == NotEqual);

We are (slowly) moving away from JS_ASSERT, in direction of MOZ_ASSERT.
Attachment #8418066 - Flags: review?(nicolas.b.pierron) → review+
Attached patch 08-General-updates-3.patch (obsolete) — Splinter Review
Carry review from previous patch.

Removed executableCopy. Will make a separate patch.
Attachment #8418064 - Attachment is obsolete: true
Attachment #8418066 - Attachment is obsolete: true
Attachment #8418694 - Flags: review+
Attachment #8418694 - Flags: checkin?(nicolas.b.pierron)
Attached patch 09-executableCopy.patch (obsolete) — Splinter Review
Attachment #8418724 - Flags: review?(nicolas.b.pierron)
Carry review from previous patch.

Updated patch because of fuzz 1 during import.
Attachment #8418694 - Attachment is obsolete: true
Attachment #8418694 - Flags: checkin?(nicolas.b.pierron)
Attachment #8419322 - Flags: review+
Attachment #8419322 - Flags: checkin?(nicolas.b.pierron)
(In reply to Carsten Book [:Tomcat] from comment #23)
> Hi, could you provide a Try link. Suggestions for what to run if you haven't
> yet can be found here:
> https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Here is the try link:
https://tbpl.mozilla.org/?tree=Try&rev=5964504030c3
Comment on attachment 8406787 [details] [diff] [review]
01-MacroAssempler-mips-missing.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fce8c9d9c0f
Attachment #8406787 - Flags: checkin?(nicolas.b.pierron) → checkin+
Comment on attachment 8411891 [details] [diff] [review]
07-General-updates-2.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/be2ced1f55fc
Attachment #8411891 - Flags: checkin?(nicolas.b.pierron) → checkin+
Comment on attachment 8419322 [details] [diff] [review]
08-General-updates-3.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf209153f20e
Attachment #8419322 - Flags: checkin?(nicolas.b.pierron) → checkin+
Attachment #8419431 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8418724 [details] [diff] [review]
09-executableCopy.patch

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

I am forwarding the Review to Marty, as worked on the IonAssemblerBuffer.
I am not sure to understand why we need to add an executableCopy here without removing any of the existing one[1]

[1] http://dxr.mozilla.org/mozilla-central/search?q=executableCopy+path%3Ajs%2Fsrc%2Fjit%2F&case=false
Attachment #8418724 - Flags: review?(nicolas.b.pierron) → review?(mrosenberg)
Attachment #8419431 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #38)
> I am forwarding the Review to Marty, as worked on the IonAssemblerBuffer.
> I am not sure to understand why we need to add an executableCopy here
> without removing any of the existing one[1]
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/
> search?q=executableCopy+path%3Ajs%2Fsrc%2Fjit%2F&case=false

ARM uses AssemblerBufferWithConstantPool which is derived from AssemblerBuffer. Since MIPS has no need for constant pool, MIPS is using the AssemblerBuffer class. This class is missing the executableCopy method that the AssemblerBufferWithConstantPool class has.
Attachment #8418724 - Flags: review?(mrosenberg) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #38)
> Comment on attachment 8418724 [details] [diff] [review]
> 09-executableCopy.patch
> 
> Review of attachment 8418724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am forwarding the Review to Marty, as worked on the IonAssemblerBuffer.
> I am not sure to understand why we need to add an executableCopy here
> without removing any of the existing one[1]
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/
> search?q=executableCopy+path%3Ajs%2Fsrc%2Fjit%2F&case=false

The only one in the assembler buffer is in the assembler buffer with constant pools.  If they're using the assembler buffer directly, then they'd need it in the parent class as well.
Attachment #8425530 - Flags: review?(nicolas.b.pierron)
Attachment #8425530 - Flags: review?(nicolas.b.pierron) → review+
Rebased patch because it failed to apply. 
Updated: r=mrosenberg

Carry review from previous patch.
Attachment #8418724 - Attachment is obsolete: true
Attachment #8426248 - Flags: review+
Checkin is needed for patches 09, 10 and 11.

Try link is here:
https://tbpl.mozilla.org/?tree=Try&rev=38fc63d99408

The bug can be closed now.
You need to log in before you can comment on or make changes to this bug.