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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Branislav Rankov [:rankov]]

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.

Comment 1

[Branislav Rankov [:rankov]]

Attachment: MacroAssempler-mips-missing.patch

Comment 2

[Branislav Rankov [:rankov]]

Attachment: Style-fixes.patch

Comment 3

[Branislav Rankov [:rankov]]

Attachment: 01-MacroAssempler-mips-missing.patch

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.

Comment 4

[Branislav Rankov [:rankov]]

Attachment: 02-Style-fixes.patch

Comment 5

[Branislav Rankov [:rankov]]

Attachment: 03-TestOverflow.patch

Comment 6

[Branislav Rankov [:rankov]]

Attachment: 04-AutoFlushCache.patch

Comment 7

[Branislav Rankov [:rankov]]

Attachment: 05-General-updates.patch

Comment 8

[Nicolas B. Pierron [:nbp]]

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.

Comment 9

[Branislav Rankov [:rankov]]

Attachment: 05-General-updates.patch

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.

Comment 10

[Branislav Rankov [:rankov]]

Attachment: 05-General-updates.patch

Carry review from previous patch.

Parts of this patch were moved to patch Trampoline-mips.patch in bug 969375.

Comment 11

[Branislav Rankov [:rankov]]

Attachment: 06-ScaledAddress.patch

This patch came from Heiher, so I'm upstreming it with his name.

Comment 12

[Branislav Rankov [:rankov]]

Attachment: 07-General-updates-2.patch

Comment 13

[Branislav Rankov [:rankov]]

Attachment: 07-General-updates-2.patch

A small add to the patch.

Comment 14

[Branislav Rankov [:rankov]]

Attachment: 02-Style-fixes.patch

Carry review from previous patch.
Updated r=nbp.

Comment 15

[Branislav Rankov [:rankov]]

Attachment: 03-TestOverflow.patch

Carry review from previous patch.
Updated r=nbp.

Comment 16

[Branislav Rankov [:rankov]]

Attachment: 04-AutoFlushCache.patch

Carry review from previous patch.
Updated r=nbp.

Comment 17

[Branislav Rankov [:rankov]]

Attachment: 06-ScaledAddress.patch

Carry review from previous patch.
Updated r=jandem.

Comment 18

[Branislav Rankov [:rankov]]

Comment on attachment 8412664 [details] [diff] [review]
06-ScaledAddress.patch

Carry review from previous patch.

Comment 19

[Branislav Rankov [:rankov]]

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?

Comment 20

[Douglas Crosher [:dougc]]

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.

Comment 21

[Branislav Rankov [:rankov]]

Attachment: 08-General-updates-3.patch

Removed 04-AutoFlushCache.patch because of comment #20 from Douglas.

Comment 22

[Branislav Rankov [:rankov]]

Attachment: 09-test-magic-value.patch

Comment 23

[Carsten Book [:Tomcat]]

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 24

[Nicolas B. Pierron [:nbp]]

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?

Comment 25

[Nicolas B. Pierron [:nbp]]

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.

Comment 26

[Branislav Rankov [:rankov]]

Attachment: 08-General-updates-3.patch

Carry review from previous patch.

Removed executableCopy. Will make a separate patch.

Comment 27

[Branislav Rankov [:rankov]]

Attachment: 09-executableCopy.patch

Comment 28

[Branislav Rankov [:rankov]]

Attachment: 08-General-updates-3.patch

Carry review from previous patch.

Updated patch because of fuzz 1 during import.

Comment 29

[Branislav Rankov [:rankov]]

(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 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8406787 [details] [diff] [review]
01-MacroAssempler-mips-missing.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fce8c9d9c0f

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8412660 [details] [diff] [review]
02-Style-fixes.patch

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8412662 [details] [diff] [review]
03-TestOverflow.patch

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8411726 [details] [diff] [review]
05-General-updates.patch

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8412664 [details] [diff] [review]
06-ScaledAddress.patch

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8411891 [details] [diff] [review]
07-General-updates-2.patch

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

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8419322 [details] [diff] [review]
08-General-updates-3.patch

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

Comment 37

[Branislav Rankov [:rankov]]

Attachment: 10-Trampolines-follow-up.patch

Comment 38

[Nicolas B. Pierron [:nbp]]

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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5fce8c9d9c0f
https://hg.mozilla.org/mozilla-central/rev/fbfff6e64063
https://hg.mozilla.org/mozilla-central/rev/3fe658d89858
https://hg.mozilla.org/mozilla-central/rev/f89de4e6408c
https://hg.mozilla.org/mozilla-central/rev/bd7f7828dde2
https://hg.mozilla.org/mozilla-central/rev/be2ced1f55fc
https://hg.mozilla.org/mozilla-central/rev/cf209153f20e

Comment 40

[Branislav Rankov [:rankov]]

(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.

Comment 41

[Marty Rosenberg [:mjrosenb]]

(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.

Comment 42

[Branislav Rankov [:rankov]]

Attachment: 11-Update-branchTestStringTruthy.patch

Comment 43

[Branislav Rankov [:rankov]]

Attachment: 09-executableCopy.patch

Rebased patch because it failed to apply. 
Updated: r=mrosenberg

Carry review from previous patch.

Comment 44

[Branislav Rankov [:rankov]]

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.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8426248 [details] [diff] [review]
09-executableCopy.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dd97c7126ec

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8419431 [details] [diff] [review]
10-Trampolines-follow-up.patch

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

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8425530 [details] [diff] [review]
11-Update-branchTestStringTruthy.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/204bc2c5a2e5

Comment 48

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

https://hg.mozilla.org/mozilla-central/rev/9dd97c7126ec
https://hg.mozilla.org/mozilla-central/rev/d930fc7af6ac
https://hg.mozilla.org/mozilla-central/rev/204bc2c5a2e5