Bug 30144 – MIPS JIT Supports

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Chao-ying Fu]

Hi All,

  We would like to contribute MIPS JIT supports for JavaScriptCore.
"jsc" was cross-compiled by CodeSourcery toolchains which version is
"mips-linux-gnu-g++ (Sourcery G++ 4.3-204) 4.3.3".

NOTE 1: The internal function calls use "JAL" for performance.  The PLT is requird
to call library functions.  Please check this page for more info.
http://gcc.gnu.org/onlinedocs/gcc-4.4.1/gcc/MIPS-Options.html#MIPS-Options
-mplt
-mno-plt
    Assume (do not assume) that the static and dynamic linkers support PLTs
and copy relocations. This option only affects `-mno-shared -mabicalls'.
For the n64 ABI, this option has no effect without `-msym32'.
    You can make -mplt the default by configuring GCC with --with-mips-plt.
The default is -mno-plt otherwise.
------------

NOTE 2: "jsc" was compiled for little-endian 32-bit MIPS 24Kf with O32 ABI.
Other configurations (like 64-bit CPUs with n32/n64//o64 ABI) may not work.
If someone is interested in other systems, we can try to support later.

NOTE 3: "jsc" was tested on top of Debian 5.0 on a MIPS 24Kf malta board with a new
library path and a new dynamic loader (that come from CodeSourcery toolchains).

  Feedback on this patch or questions are welcome.  Thanks!

Regards,
Chao-ying

[Chao-ying Fu]

Created attachment 40751 [details]
MPIS JIT patch

[Gavin Barraclough]

Hi Chao-ying,

Thanks for the patch, and from a really quick glance through it looks like a great port.

I think I'm likely to be the person reviewing this (unless Geoff, Olliej or Sam feel like jumping in).  Apologies, but this week is rather busy for me and a patch like this will take a while to get through.  I'm afraid you're probably not going to see a full response to this until next week.

cheers,
G.

[Mark Rowe (bdash)]

Two minor comments to start with, one of which will make it much easier to review the patch:
1) We use four spaces for indentation rather than hard tabs.  The patch mixes both spaces and tabs.  The mix of tabs and spaces in the patch makes the indentation very hard to follow.  You can see this by clicking on the “Formatted Diff” link above.
2) The copyright notice and license header should match the file that you based your work on.  In particular the “All rights reserved.” statement has been removed from the Apple copyright notice, and one file refers to "UNIVERSITY OF SZEGED” but lacks the copyright notice which the reference implies should be present.

[Mark Rowe (bdash)]

(In reply to comment #3)
> 2) The copyright notice and license header should match the file that you based
> your work on.  In particular the “All rights reserved.” statement has been
> removed from the Apple copyright notice, and one file refers to "UNIVERSITY OF
> SZEGED” but lacks the copyright notice which the reference implies should be
> present.

Looking further it seems that this is based on either the x86 or ARMv7 versions of the assembler and macro assembler, in which case the reference to the University of Szeged should not be present.  You should copy the license body from the file on which the derived file is based.

[Gabor Loki]

> +    #define ENABLE_JIT 1

I think you should add only JIT functionality first which is disabled by default. After JIT is landed a second patch can enable it.

> +    #define ENABLE_JIT_OPTIMIZE_CALL 1
> +    #define ENABLE_JIT_OPTIMIZE_NATIVE_CALL 1
> +    #define ENABLE_JIT_OPTIMIZE_PROPERTY_ACCESS 1
> +    #define ENABLE_JIT_OPTIMIZE_METHOD_CALLS 1

You can skip those. Each optimization will be enabled later, except it is disabled by hand.

>  #define ENABLE_YARR 1
>  #define ENABLE_YARR_JIT 1

Every big patch can be reviewed and/or tested easier if it is split into several part :)
for example:
- JIT only for YARR
- the remaining code of JIT
- enable YARR_JIT for specific PLATFORM
- enable JIT as well

> + * Copyright (C) 2009 Apple Inc.
> + * Copyright (C) 2009 MIPS Technologies, Inc.
> + * All rights reserved.
...
> + * THIS SOFTWARE IS PROVIDED BY UNIVERSITY OF SZEGED ``AS IS'' AND ANY

You should add the copyright line for University of Szeged as well. This approach is based on ARMAssembler:
- using 'emitInst' concept
- there is no 'm_formatter'

> +	// Check each jump
> +	for(Jumps::Iterator iter = m_jumps.begin(); iter != m_jumps.end(); ++iter) {
> +	    int pos = (*iter);
> +	    MIPSWord *insn = reinterpret_cast<MIPSWord*>(reinterpret_cast<intptr_t>(m_buffer.data()) + pos);
> +	    insn = insn + 2;
> +	    if(((*insn) & 0xfc000000) == 0x08000000) {
> +		int offset = (*insn) & 0x03ffffff;
> +		int old_insn_address = (int)insn;
> +		int top_four_bits = (old_insn_address + 4) >> 28;
> +		int old_target_address = (top_four_bits << 28) | (offset << 2);
> +		int old_base = (int)m_buffer.data();
> +		int new_base = (int)result;
> +		int new_target_address = old_target_address - old_base + new_base;
> +		int new_insn_address = old_insn_address - old_base + new_base;
> +		if (((new_insn_address + 4) >> 28)
> +		    != (new_target_address >> 28)) {
> +		    fprintf (stderr, "Error: Cannot create MIPS jump (J) due to the top four bits are different.\n");
> +		}
> +		*insn = 0x08000000 | ((new_target_address >> 2) & 0x3ffffff);
> +	    }

What happens if a jump cannot be created? Crash?

> +	    insn = insn + 2;
> +	    if (((reinterpret_cast<intptr_t>(insn) + 4) >> 28)
> +		!= (reinterpret_cast<intptr_t>(to) >> 28)) {
> +		fprintf(stderr, "Error: Cannot create MIPS jump (J) due to the top four bits are different.\n");
> +		CRASH();

Is this approach working anyway? Is there no possible way to jump to an address which is held by a 32bits register? (I am not familiar with MIPS)
I think you should add some kind of workaround for this case (for example: using constant pool?).

> +    AssemblerBuffer m_trampolineBuffer;

No one uses!

> +    bool supportsFloatingPoint() const
> +    {
> +#if defined(__mips_hard_float) && !defined(__mips_single_float)
> +	return true;
> +#else
> +	return false;
> +#endif
> +    }

It would be better if some kind of on-the-fly detection is used in here.
For example: see isVFPPresent() in MacroAssemblerARM.cpp.

> +	__clear_cache(reinterpret_cast<char*>(start), reinterpret_cast<char*>(end));

You should avoid __clear_cache. Although it is exported from GCC, this is still an internal function. See my comments on https://bugs.webkit.org/show_bug.cgi?id=28886#c35 bug.

Well, you can still use __clear_cache if you guard this call with several GCC versions. For example:
#elif PLATFORM(MIPS) && COMPILER(GCC) && (GCC_VERSION == 40201 || GCC_VERSION == 40304)

Although I am not a reviewer, these were my comments on MIPS JIT.
Anyway, we know this task was not an easy one. Congratulations! Keep going on this way! :)

[Zoltan Herczeg]

Is that patch only for mips32? Somehow you should prepare for the 64 bit systems as well to avoid a lot of changes in defines. (And drop a #error message now, if 64 bit mips is the target architecture)

[Chao-ying Fu]

(In reply to comment #6)
> Is that patch only for mips32? Somehow you should prepare for the 64 bit
> systems as well to avoid a lot of changes in defines. (And drop a #error
> message now, if 64 bit mips is the target architecture)

The port is for 32-bit MIPS with O32 ABI.  If we compile for 64-bit MIPS, the JIT will not be enabled in "Platform.h" due to different ABI.  We can work on 64-bit MIPS, after the 32-bit MIPS port is accepted.  Thanks!

[Chao-ying Fu]

(In reply to comment #4)
> (In reply to comment #3)
> > 2) The copyright notice and license header should match the file that you based
> > your work on.  In particular the “All rights reserved.” statement has been
> > removed from the Apple copyright notice, and one file refers to "UNIVERSITY OF
> > SZEGED” but lacks the copyright notice which the reference implies should be
> > present.
> 
> Looking further it seems that this is based on either the x86 or ARMv7 versions
> of the assembler and macro assembler, in which case the reference to the
> University of Szeged should not be present.  You should copy the license body
> from the file on which the derived file is based.

MIPSAssembler.h is based on X86, ARM, ARMv7 files.  I can include all copyright lines. Ex:
 * Copyright (C) 2008 Apple Inc. All rights reserved.
 * Copyright (C) 2009 Apple Inc. All rights reserved.
 * Copyright (C) 2009 University of Szeged
 * Copyright (C) 2009 MIPS Technologies Inc.

For this line:
 * THIS SOFTWARE IS PROVIDED BY ??????? ``AS IS'' AND ANY

Should I use Apple or Univ of Szeged?  Thanks!

[Zoltan Herczeg]

I would put your copyright into the first line, since you did the work. And "PROVIDED BY your company." of course. Just not forget to mention the others :)

[Chao-ying Fu]

(In reply to comment #5)
> > +    #define ENABLE_JIT 1
> 
> I think you should add only JIT functionality first which is disabled by
> default. After JIT is landed a second patch can enable it.
> 
> > +    #define ENABLE_JIT_OPTIMIZE_CALL 1
> > +    #define ENABLE_JIT_OPTIMIZE_NATIVE_CALL 1
> > +    #define ENABLE_JIT_OPTIMIZE_PROPERTY_ACCESS 1
> > +    #define ENABLE_JIT_OPTIMIZE_METHOD_CALLS 1
> 
> You can skip those. Each optimization will be enabled later, except it is
> disabled by hand.

  Yes.

> Every big patch can be reviewed and/or tested easier if it is split into
> several part :)
> for example:
> - JIT only for YARR
> - the remaining code of JIT
> - enable YARR_JIT for specific PLATFORM
> - enable JIT as well

  The main part is two files: MIPSAssembler.h and MacroAssemblerMIPS.h.  Other files are only modified to define something for MIPS.
> 
> > + * Copyright (C) 2009 Apple Inc.
> > + * Copyright (C) 2009 MIPS Technologies, Inc.
> > + * All rights reserved.
> ...
> > + * THIS SOFTWARE IS PROVIDED BY UNIVERSITY OF SZEGED ``AS IS'' AND ANY
> 
> You should add the copyright line for University of Szeged as well. This
> approach is based on ARMAssembler:
> - using 'emitInst' concept
> - there is no 'm_formatter'

Yes.


> 
> > +	// Check each jump
> > +	for(Jumps::Iterator iter = m_jumps.begin(); iter != m_jumps.end(); ++iter) {
> > +	    int pos = (*iter);
> > +	    MIPSWord *insn = reinterpret_cast<MIPSWord*>(reinterpret_cast<intptr_t>(m_buffer.data()) + pos);
> > +	    insn = insn + 2;
> > +	    if(((*insn) & 0xfc000000) == 0x08000000) {
> > +		int offset = (*insn) & 0x03ffffff;
> > +		int old_insn_address = (int)insn;
> > +		int top_four_bits = (old_insn_address + 4) >> 28;
> > +		int old_target_address = (top_four_bits << 28) | (offset << 2);
> > +		int old_base = (int)m_buffer.data();
> > +		int new_base = (int)result;
> > +		int new_target_address = old_target_address - old_base + new_base;
> > +		int new_insn_address = old_insn_address - old_base + new_base;
> > +		if (((new_insn_address + 4) >> 28)
> > +		    != (new_target_address >> 28)) {
> > +		    fprintf (stderr, "Error: Cannot create MIPS jump (J) due to the top four bits are different.\n");
> > +		}
> > +		*insn = 0x08000000 | ((new_target_address >> 2) & 0x3ffffff);
> > +	    }
> 
> What happens if a jump cannot be created? Crash?

  Yes, if we cannot use "J", it will crash.
But this type of jump can cover a 28-bit range of targets.  From running sunspider and v8 tests, 28-bit jumps seem ok.

> 
> > +	    insn = insn + 2;
> > +	    if (((reinterpret_cast<intptr_t>(insn) + 4) >> 28)
> > +		!= (reinterpret_cast<intptr_t>(to) >> 28)) {
> > +		fprintf(stderr, "Error: Cannot create MIPS jump (J) due to the top four bits are different.\n");
> > +		CRASH();
> 
> Is this approach working anyway? Is there no possible way to jump to an address
> which is held by a 32bits register? (I am not familiar with MIPS)
> I think you should add some kind of workaround for this case (for example:
> using constant pool?).

  If we do need 32-bit jumps, we can create Jump Register ("JR") with "LUI + ORI" for 32-bit target addresses.  This will cost four extra instructions, compared to two extra instructions for Jump ("J").  This is not implemented, however.
  I will think about using constant pool to make the handling of branches more robust.

> 
> > +    AssemblerBuffer m_trampolineBuffer;
> 
> No one uses!

  Yes.  I will delete it.


> 
> > +    bool supportsFloatingPoint() const
> > +    {
> > +#if defined(__mips_hard_float) && !defined(__mips_single_float)
> > +	return true;
> > +#else
> > +	return false;
> > +#endif
> > +    }
> 
> It would be better if some kind of on-the-fly detection is used in here.
> For example: see isVFPPresent() in MacroAssemblerARM.cpp.

  I will try to query the system to know if floating-point can be used.
For now, I assume users know their systems to provide compiler flags.

> 
> > +	__clear_cache(reinterpret_cast<char*>(start), reinterpret_cast<char*>(end));
> 
> You should avoid __clear_cache. Although it is exported from GCC, this is still
> an internal function. See my comments on
> https://bugs.webkit.org/show_bug.cgi?id=28886#c35 bug.
> 
> Well, you can still use __clear_cache if you guard this call with several GCC
> versions. For example:
> #elif PLATFORM(MIPS) && COMPILER(GCC) && (GCC_VERSION == 40201 || GCC_VERSION
> == 40304)

  Yes.  I will add the guard to check GCC and its version.

> Although I am not a reviewer, these were my comments on MIPS JIT.
> Anyway, we know this task was not an easy one. Congratulations! Keep going on
> this way! :)

  Thanks a lot for your review.  :-)

[Chao-ying Fu]

Created attachment 40825 [details]
MIPS JIT Patch 20091007

  I updated the patch with space instead of tab,
and updated copyright headers for new files.
Minor changes in some files are based on the review.  Thanks!

[David Kilzer (:ddkilzer)]

Comment on attachment 40825 [details]
MIPS JIT Patch 20091007

(In reply to comment #11)
> Created an attachment (id=40825) [details]
> MIPS JIT Patch 20091007
> 
>   I updated the patch with space instead of tab,
> and updated copyright headers for new files.
> Minor changes in some files are based on the review.  Thanks!

Please set the "review" flag to "?" (not "+") if you want it to be reviewed.

Setting the "review" flag to "+" means the patch has been reviewed by a specific WebKit reviewer and is ready to be committed.  More details about WebKit reviewers here:  <http://webkit.org/coding/commit-review-policy.html>

[Gabor Loki]

(In reply to comment #10)
> > > +		    fprintf (stderr, "Error: Cannot create MIPS jump (J) due to the top four bits are different.\n");
> > > +		}
> > > +		*insn = 0x08000000 | ((new_target_address >> 2) & 0x3ffffff);
> > > +	    }
> > 
> > What happens if a jump cannot be created? Crash?
> 
>   Yes, if we cannot use "J", it will crash.
> But this type of jump can cover a 28-bit range of targets.  From running
> sunspider and v8 tests, 28-bit jumps seem ok.

The JavaScriptCore tests are essential. Did you test with ./WebKitTools/Scripts/run-javascriptcore-tests?

In additional, you should test at least with ./WebKitTools/Scripts/run-webkit-tests LayoutTests/fast/js as well! The new JS regression tests were landed there.

[Chao-ying Fu]

(In reply to comment #13)
> (In reply to comment #10)
> > > > +		    fprintf (stderr, "Error: Cannot create MIPS jump (J) due to the top four bits are different.\n");
> > > > +		}
> > > > +		*insn = 0x08000000 | ((new_target_address >> 2) & 0x3ffffff);
> > > > +	    }
> > > 
> > > What happens if a jump cannot be created? Crash?
> > 
> >   Yes, if we cannot use "J", it will crash.
> > But this type of jump can cover a 28-bit range of targets.  From running
> > sunspider and v8 tests, 28-bit jumps seem ok.
> 
> The JavaScriptCore tests are essential. Did you test with
> ./WebKitTools/Scripts/run-javascriptcore-tests?
> 
> In additional, you should test at least with
> ./WebKitTools/Scripts/run-webkit-tests LayoutTests/fast/js as well! The new JS
> regression tests were landed there.

Thank you for the testing information!  I will run these two tests and verify the correctness.

[Chao-ying Fu]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #10)
> > > > > +		    fprintf (stderr, "Error: Cannot create MIPS jump (J) due to the top four bits are different.\n");
> > > > > +		}
> > > > > +		*insn = 0x08000000 | ((new_target_address >> 2) & 0x3ffffff);
> > > > > +	    }
> > > > 
> > > > What happens if a jump cannot be created? Crash?
> > > 
> > >   Yes, if we cannot use "J", it will crash.
> > > But this type of jump can cover a 28-bit range of targets.  From running
> > > sunspider and v8 tests, 28-bit jumps seem ok.
> > 
> > The JavaScriptCore tests are essential. Did you test with
> > ./WebKitTools/Scripts/run-javascriptcore-tests?
> > 
> > In additional, you should test at least with
> > ./WebKitTools/Scripts/run-webkit-tests LayoutTests/fast/js as well! The new JS
> > regression tests were landed there.
> 
> Thank you for the testing information!  I will run these two tests and verify
> the correctness.

I finished ./WebKitTools/Scripts/run-javascriptcore-tests .
The results are as follows.
        ecma/GlobalObject/15.1.2.3-2.js
        ecma_3/RegExp/regress-72964.js
        js1_5/Regress/regress-159334.js

3 regressions found.
0 tests fixed.

I debugged the first two tests and fixed them.
The first test failed due to that we need to relocate MIPS jumps after the buffer grows.
The second test failed due to that we need to use zero-extended load to load 16-bit data.
The third test failed under both non-JIT and JIT versions.  I will debug it next week.

I will attached a new patch that fixed the first two tests.  Thanks!

[Chao-ying Fu]

Created attachment 40971 [details]
MIPS JIT Patch 20091009

This patch fixed two regressions.
ecma/GlobalObject/15.1.2.3-2.js
ecma_3/RegExp/regress-72964.js

[Chao-ying Fu]

Created attachment 41075 [details]
MIPS JIT Patch 20091012

I svn-updated my JavaScriptCore directory and generated a new patch.

Changes in MacroAssemblerMIPS.h:
1. Added unaligned load and branch functions.
2. Fixed LUI to use (offset + 0x8000)>>16 in many places.
3. Used m_isPointer to make sure that two instructions are generated.
4. Used move() functions in all places.

Changes in MIPSAssembler.h:
1. Added lwl, lwr.
2. Fixed relocateJumps() to check pos that has 3 more instructions.

The testing showed new regressions as follows.
        ecma/Date/15.9.5.10-10.js
        ecma/Date/15.9.5.10-2.js
        ecma/Date/15.9.5.10-6.js
        ecma/Date/15.9.5.10-7.js
        ecma/Date/15.9.5.10-8.js
        ecma/Date/15.9.5.10-9.js
        ecma/Date/15.9.5.25-1.js
        ecma/Date/15.9.5.26-1.js
        ecma/Date/15.9.5.28-1.js
        ecma/Date/15.9.5.30-1.js
        ecma/Date/15.9.5.31-1.js
        ecma/Date/15.9.5.34-1.js
        ecma/Date/15.9.5.36-4.js
        ecma/Date/15.9.5.36-5.js
        ecma/Date/15.9.5.36-6.js
        ecma/Date/15.9.5.36-7.js
        ecma/Date/15.9.5.37-4.js
        ecma/Date/15.9.5.37-5.js

I need to debug it later.  Thanks!

[Eric Seidel (no email)]

I used to joke about someone porting WebKit to MIPS.  I'm not sure if I should laugh or cry about this bug.  Either way, Oliver, Gavin or Geoff are going to be your best reviewers for JIT code in the end.

[Chao-ying Fu]

Created attachment 41741 [details]
MIPS JIT Patch 20091022

I updated the patch with some changes.

1. Added better PIC code handling in the trampoline.
2. Removed CRASH() by supporting 32-bit jumps and function calls.
3. Cleaned up source files according to the coding style.

Thanks!

[Oliver Hunt]

Comment on attachment 41741 [details]
MIPS JIT Patch 20091022

I am just doing a cursory review -- Gavin should probably look at this in more detail.

I find the comments thaat repeat the funciton name to be superfluous, eg
 257     void lui(RegisterID rt, int imm)
 258     {
 259         /* lui */
 260         emitInst(0x3c000000 | (rt << OP_SH_RT) | (imm & 0xffff));
 261     }

The comment /* lui */ contains no useful information so should be removed.

Same applies for all the other cases (addiu, addu, subu, ... )

I don't really like the repeated 
 #if __mips == 1
         nop();
 #endif

And would prefer a function akin to

void loadDelayNop()
{
#if __mips == 1
         nop();
#endif
}

Or whatever the name should be (i'm guessing about the name, it's been years since i did anything at all with mips) to give it most clarity, and then replace those conditional nops with an anconditional call to the helper function.

I have to disappear for a bit -- i'm up to assembler/MacroAssemblerMIPS.h -- overall this patch looks really good though.  Possibly the first large patch from a new contributor to have (so far) perfect code style :D

--Oliver

[Chao-ying Fu]

(In reply to comment #20)
> (From update of attachment 41741 [details])
> I am just doing a cursory review -- Gavin should probably look at this in more
> detail.
> 
> I find the comments thaat repeat the funciton name to be superfluous, eg
>  257     void lui(RegisterID rt, int imm)
>  258     {
>  259         /* lui */
>  260         emitInst(0x3c000000 | (rt << OP_SH_RT) | (imm & 0xffff));
>  261     }
> 
> The comment /* lui */ contains no useful information so should be removed.
> 
> Same applies for all the other cases (addiu, addu, subu, ... )

  Yes.  I will remove all the cases in the new patch.

> 
> I don't really like the repeated 
>  #if __mips == 1
>          nop();
>  #endif
> 
> And would prefer a function akin to
> 
> void loadDelayNop()
> {
> #if __mips == 1
>          nop();
> #endif
> }
> 
> Or whatever the name should be (i'm guessing about the name, it's been years
> since i did anything at all with mips) to give it most clarity, and then
> replace those conditional nops with an anconditional call to the helper
> function.

  This is a good idea.  I will create loadDelayNop() and copDelayNop() for mips1 in the new patch.

> 
> I have to disappear for a bit -- i'm up to assembler/MacroAssemblerMIPS.h --
> overall this patch looks really good though.  Possibly the first large patch
> from a new contributor to have (so far) perfect code style :D
> 
> --Oliver

  Thanks a lot for your review!

[Chao-ying Fu]

Created attachment 42114 [details]
MIPS JIT Patch 20091029

I updated the patch based on the review to remove redundant comments
and to use loadDelayNop() and copDelayNop().

Other changes in this patch:
1. For __clear_cache, we test (GCC_VERSION < 40500) to perform the workaround.
Since GCC 4.5 has the fix for MIPS, we don't need the workaround.

2. In linkWithOffset, we check "*(insn + 2) != 0x10000003" to generate j or lui/ori/jr
for correctness.

3. Extra () are removed.

[Gavin Barraclough]

Comment on attachment 42114 [details]
MIPS JIT Patch 20091029

Hi Chao-ying,

First of all, many apologies for the delay in looking at this – busy couple of weeks.  This looks like a great patch.

As I think Gabor or Zoltan may have already commented, we have usually landed new ports incrementally to try to separate new code from changes to existing code.  This both makes it easier to review, and makes it easier to track down problems later if change were to introduce a problem.  I'd suggest it would be good to do so in this case too – it should not significantly slow down the progress of getting this in.  So if you could put up a patch for review that reverts all changes to the JIT (and obviously also doesn't set ENABLE_JIT!) that would be great – the first patch should just enable the YARR regex engine on MIPS.

In the past we have also broken up the patch that enables the JIT to separately enable the JIT without optimization, then enable the optimizations one by one.  This may not be necessary this time, since landing the last couple of ports also required changes to make the JIT more portable – the diff needed to enable the JIT on MIPS looks small enough that we can probably land the rest as one piece.


> Index: wtf/Platform.h
> ===================================================================
> --- wtf/Platform.h	(revision 49926)
> +++ wtf/Platform.h	(working copy)
>
> @@ -225,6 +225,15 @@
>   || (PLATFORM(X86_64) && PLATFORM(MAC)) \
>   /* Under development, temporarily disabled until 16Mb link range limit in assembler is fixed. */ \
>   || (PLATFORM(ARM_THUMB2) && PLATFORM(IPHONE) && 0) \
> - || (PLATFORM(X86) && PLATFORM(WIN))
> + || (PLATFORM(X86) && PLATFORM(WIN)) \
> + || PLATFORM(MIPS)
>  #define ENABLE_YARR 1
>  #define ENABLE_YARR_JIT 1
>  #endif

We tend to only enable the JIT platform by platform, and only on platforms we've tested.  Also, the Linux ports don't enable features in Platform.h – I think GTK & QT ports use the files /configure.ac and /JavaScriptCore/JavaScriptCore.pri.  What platforms have you been testing on? - if linux, I'd suggest sticking with the other Linux ports.  If other, feel free to make the change here in Platform.h, but only do so for specific OSes.

> Index: assembler/MIPSAssembler.h
> ===================================================================
> +    JmpSrc jmp()
> +    {
> +        m_jumps.append(m_buffer.size());
> +
> +        beq(MIPSRegisters::zero, MIPSRegisters::zero, 0);
> +        nop();
> +        /* We need four words for relaxation.  */
> +        beq(MIPSRegisters::zero, MIPSRegisters::zero, 3); // Jump over nops
> +        nop();
> +        nop();
> +        nop();
> +        return JmpSrc(m_buffer.size());
> +    }
> +
> +    JmpSrc branch_eq(RegisterID rs, RegisterID rt)
> +    {
> +        m_jumps.append(m_buffer.size());
> +
> +        beq(rs, rt, 0);
> +        nop();
> +        /* We need four words for relaxation.  */
> +        beq(MIPSRegisters::zero, MIPSRegisters::zero, 3); // Jump over nops
> +        nop();
> +        nop();
> +        nop();
> +        return JmpSrc(m_buffer.size());
> +    }
> +
> +    JmpSrc branch_ne(RegisterID rs, RegisterID rt)
> +    {
> +        m_jumps.append(m_buffer.size());
> +
> +        bne(rs, rt, 0);
> +        nop();
> +        /* We need four words for relaxation.  */
> +        beq(MIPSRegisters::zero, MIPSRegisters::zero, 3); // Jump over nops
> +        nop();
> +        nop();
> +        nop();
> +        return JmpSrc(m_buffer.size());
> +    }
> +
> +    JmpSrc bc1t()
> +    {
> +        emitInst(0x45010000);
> +        nop();
> +        /* We need four words for relaxation.  */
> +        beq(MIPSRegisters::zero, MIPSRegisters::zero, 3); // Jump over nops
> +        nop();
> +        nop();
> +        nop();
> +        return JmpSrc(m_buffer.size());
> +    }
> +
> +    JmpSrc far_function_call()
> +    {
> +        lui(MIPSRegisters::t9, 0);
> +        ori(MIPSRegisters::t9, MIPSRegisters::t9, 0);
> +        jalr(MIPSRegisters::t9);
> +        nop();
> +        return JmpSrc(m_buffer.size());
> +    }
> +
> +    JmpSrc function_call()
> +    {
> +        /* We need two words for relaxation.  */
> +        nop();
> +        nop();
> +        jal();
> +        nop();
> +        return JmpSrc(m_buffer.size());
> +    }

These shouldn't really be in the Assembler layer.  The goal behind the Assembler is that this should really have a 1-to-1 mapping to the instruction set, and the MacroAssembler should support a richer set of operations, common across architectures.  We're not entirely strict about this, and I think from a practical perspective having the assembler automatically insert nops after loads and coprocessor moves on MIPS1 is probably not a terrible idea – though even this I'm not 100% comfortable about, this could be in the MacroAssembler too.  But the assembler should really be usable to plant jumps for use with code generation other than from the MacroAssembler, and in contexts where all code will be known to fall within a smaller region of memory, and jumps need never be relaxed to 32-bit range.  So I'd suggest these functions should really be moved up to the MacroAssembler.

Unfortunately the layering is currently a little broken in the assemblers for other architectures, in this area.  The linking and repatching of jumps and calls currently is handled by the Assembler in a manner that has knowledge of the instruction sequences used by the MacroAssembler.  We've started refactoring the code to fix this, so the linking and repatching now passes through the MacroAssemblerARCH layer, and the intention is that all knowledge of the set of instructions used by the MacroAssembler will be captured here – so, for example note that MacroAssemblerX86_64::linkCall is already responsible for offseting the label to find the immediate to patch for far calls.  But cleaning all this up is clearly a work in progress.


> +    void* executableCopy(ExecutablePool* allocator)
> +    {
> +        int size = m_buffer.size();
> +        if (!size)
> +            return 0;
> +
> +        void* result = allocator->alloc(size);
> +        if (!result)
> +            return 0;
> +
> +        ExecutableAllocator::makeWritable(result, size);
> +        memcpy(result, m_buffer.data(), m_buffer.size());
>

^^^ This seems to just replicate functionality for m_buffer.executableCopy(allocator); is there a reason you can't just call that?

 +        relocateJumps(m_buffer.data(), result);
> +        return result;
> +    }


In MacroAssemblerARMv7.h ->

>> , int fixed = 0)

The one big problem I have in this patch is adding the ", int fixed = 0)" argument to all these function.  We shouldn't be adding an extra parameter to these functions on one platform – we really need this interface to line up across the architectures (this interface is not fixed, and if it differs across platforms it will be difficult to change).  I'd suggest this can be changed really easily, by just using a member variable - add a member bool m_fixedWidthImmedaites, initialize this to false, set it to true in the places you set fixed = 1;, and check m_fixedWidthImmedaites instead of fixed in the places you check the value.

> +    Call tailRecursiveCall()
> +    {
> +        // Like a normal call, but don't link.

Whilst I understand this is completely correct, I think this comment has the potential to be confusing, since in the context of calls we're usually use to word 'link' to mean static link the JIT generated code.  Being explicit and saying something like "but don't set the link register" might just avoid any confusion.

> Index: jit/ExecutableAllocator.h
> ===================================================================
> +        int start = reinterpret_cast<intptr_t>(code) & (-line_size);
"int foo = reinterpret_cast<intptr_t>" should probably be "intptr_t foo = reinterpret_cast<intptr_t>". :-)


I'll take another look over the whole patch when you're addressed these issues, but nothing else stood out to me, all looks good otherwise.

cheers,
G.

[Chao-ying Fu]

Hi Gavin,

  Thanks for your review!  I will try to address the issues you raised.
And I will split the patch into parts: YARR, YARR_JIT, JIT without optimization, JIT with optimizations.  I am busy this week, so maybe next week I will post new patches.  Thanks again!

Regards,
Chao-ying

[Chao-ying Fu]

Created attachment 42603 [details]
MIPS Patch Part 1 for YARR

Hi,

  I started to split the patch.  Here is the first patch to enable YARR.
The MIPS platform was tested with Qt on top of Debian Linux.
Thus, MIPS defines are inside PLATFORM(QT).
Note: ENABLE_YARR_JIT is 0 and ENABLE_JIT is 0.
Thanks!

Regards,
Chao-ying

[Chao-ying Fu]

Created attachment 42608 [details]
MIPS Patch Part 1 for YARR (new)

The previous patch put the test into a wrong location to enable YARR for MIPS.
Fixed in the new patch.  Thanks!

[Gavin Barraclough]

Just a quick warning, we're now specified that the MacroAssembler shift methods must clamp the shift amount to the range 0..31 - see http://trac.webkit.org/changeset/50595 .

On x86 this requires no additional code, since the operand to the shift is implicitly clamped to this range.  On the ARM platforms we plant an extra arm instruction – I don't know what the specified behaviour is on MIPS, you may need to do the same?

[Chao-ying Fu]

(In reply to comment #27)
> Just a quick warning, we're now specified that the MacroAssembler shift methods
> must clamp the shift amount to the range 0..31 - see
> http://trac.webkit.org/changeset/50595 .
> 
> On x86 this requires no additional code, since the operand to the shift is
> implicitly clamped to this range.  On the ARM platforms we plant an extra arm
> instruction – I don't know what the specified behaviour is on MIPS, you may
> need to do the same?

On MIPS, the shift amount is clamped into 0..31 (from bit 0 to bit 4 of a rs register) automatically.  So, no extra code is required.  Thanks!

[Chao-ying Fu]

Created attachment 42679 [details]
MIPS JIT Patch 20091106

  I ran "svn update" and fixed some issues today.

ChangeLog:
1. Moved some code from MIPSAssembler.h to MacroAssemblerMIPS.h,
   based on Gavin's review.
2. Removed the parameter (int fixed = 0) in MacroAssemblerMIPS.h.
   Created "bool m_fixedWidth", instead.
3. Updated DoubleCondition to have *Unordered.
4. Updated cacheFlush() to call _flush_cache(), when __clear_cache()
   cannot be provided by GCC which version is under version 4.3.0.
5. Made some other changes based on Gavin's review.

  The only remaining issue is to move linkCall to MacroAssemblerMIPS.h.

  I know that I still need to create small patches to be accepted step by step.
Thanks!

Regards,
Chao-ying

[Gavin Barraclough]

Comment on attachment 42608 [details]
MIPS Patch Part 1 for YARR (new)

Hey Chao-ying,

Sorry for the delay, I'm going to r- this, let me explain why.

I'm guessing that you're assuming (quite reasonably) that YARR-JIT is an extension to YARR, but this is not currently the case (although, slightly confusingly, YARR_JIT does require YARR to be defined. :-/).  While both share some common underpinnings, the two are currently used exclusively.  We hope to use YARR to replace use of our port of PCRE on platforms for which we don't have JIT support (an ever diminishing number, once MIPS joins the fold with JIT support!) - however the YARR interpreter is currently a performance regression versus PCRE.  YARR-JIT currently supports a subset of the JS-regex syntax, and may need to fallback to use an interpreter to execute certain regular expressions.  However due to PCRE having a higher performance we currently fallback to PCRE rather than YARR (interpreter).

It is our intention both to complete YARR-JIT, so there is no fallback, and also to improve the performance of YARR (interpreter), so that PCRE can be fully deprecated.

For the time being, I suggest you skip over this step of enabling YARR without the JIT, and go straight to enabling YARR-JIT.

cheers,
G.

[Gavin Barraclough]

Comment on attachment 42679 [details]
MIPS JIT Patch 20091106

This patch basically looks good to go to me – other than the fact that I'd still like to see the JIT changes (other than jit/ExecutableAllocator.h)  split out and landed in a separate patch.

My only comments relate to a couple of empty if-clauses in your patch:

> +    void or32(Imm32 imm, RegisterID dest)
> +    {
> +        if (!imm.m_isPointer && imm.m_value == 0 && !m_fixedWidth)
> +            ;
> +        else if (!imm.m_isPointer && imm.m_value > 0 && imm.m_value < 65535
> +                 && !m_fixedWidth)
> +            m_assembler.ori(dest, dest, imm.m_value);
> +        else {
> +            /*
> +                li      dataTemp, imm
> +                or      dest, dest, dataTemp
> +            */
> +            move(imm, dataTempRegister);
> +            m_assembler.or_insn(dest, dest, dataTempRegister);
> +        }
> +    }

In webkit we tend to favour early returns in complex ifs like this, something more like:

    if (!imm.m_isPointer && imm.m_value == 0 && !m_fixedWidth)
        return;

    if (!imm.m_isPointer && imm.m_value > 0 && imm.m_value < 65535 && !m_fixedWidth) {
        m_assembler.ori(dest, dest, imm.m_value);
        return;
    }

    /*
        li      dataTemp, imm
        or      dest, dest, dataTemp
    */
    move(imm, dataTempRegister);
    m_assembler.or_insn(dest, dest, dataTempRegister);

> +    void move(RegisterID src, RegisterID dest)
> +    {
> +        if (src == dest && !m_fixedWidth)
> +            ;
> +        else
> +            m_assembler.move(dest, src);
> +    }

Same here (and two other functions also), or just invert the logic in the if & do away with the else case.

Other than these minor quibbles the non-JIT code all looks fine to me, but this is a lot of fresh code, so I've asked oliver to pass a second set of eyes over it.

cheers,
G.

[Chao-ying Fu]

Created attachment 44104 [details]
MIPS JIT Patch 20091201

This is the complete patch.

ChangeLog:
1. In MacroAssemblerMIPS.h: updated code based on Gavin's review.
2. In MacroAssemblerMIPS.h: fixed a bug in branchMul32(Condition cond, Imm32 imm, RegisterID src, RegisterID dest).
3. In ExecutableAllocator.h: fixed "end" for GCC synci bug workaround.

The part 1 (YARR and YARR_JIT) of this patch will be posted soon.

[Chao-ying Fu]

Created attachment 44106 [details]
MIPS Patch Part 1 20091201

This Part 1 patch enables YARR and YARR_JIT.  Thanks!

[Oliver Hunt]

Comment on attachment 44106 [details]
MIPS Patch Part 1 20091201

Gavin, this patch looks good to me -- i would be willing to r+ it but i'm not sure if you wanted to give it a once over

--Oliver

[amol63841]

hi Chao-ying,

Thanks for the patch, I applied this patch in QT4.6 webkit jsc. It is found that CPU used in this version on webkit in more that compared to version without JIT patch. I am having some questions regarding this. I am using "mips-linux-g++" without -mplt
-mno-plt options enabled.

1. What will be the reason for increased CPU usage ?
2. Can i find any bench marking method for performance computation ?
3. What section of webkit i should see for performances improvement ?


regards,
Amol

[Chao-ying Fu]

(In reply to comment #35)
> hi Chao-ying,
> 
> Thanks for the patch, I applied this patch in QT4.6 webkit jsc. It is found
> that CPU used in this version on webkit in more that compared to version
> without JIT patch. I am having some questions regarding this. I am using
> "mips-linux-g++" without -mplt
> -mno-plt options enabled.
> 
> 1. What will be the reason for increased CPU usage ?
I am not sure about this.  Maybe CPU is busier to produce JIT instructions at run time.

> 2. Can i find any bench marking method for performance computation ?
You can try SunSpider benchmark to check if JIT can shorten the overall time.
Use the demo browser to view http://www2.webkit.org/perf/sunspider-0.9/sunspider.html and click "Start Now".

> 3. What section of webkit i should see for performances improvement ?
You should see JavaScript performance improvement from webkit.

Thanks for trying the patch!

Regards,
Chao-ying

[Eric Seidel (no email)]

Comment on attachment 44104 [details]
MIPS JIT Patch 20091201

This patch fails to apply and thus cannot be landed w/o an update.

[Eric Seidel (no email)]

Comment on attachment 44106 [details]
MIPS Patch Part 1 20091201

This patch fails to apply and thus cannot be landed without an update.  It also looks like it may be missing a ChangeLog.

[Chao-ying Fu]

Created attachment 47887 [details]
MIPS JIT Patch 20100201

I updated the patch and added ChangeLog.
The test result is:
debian:/disk/fu/dev/WebKit# ./WebKitTools/Scripts/run-javascriptcore-tests --root=/disk/fu/dev/WebKit/WebKitBuild/Release/JavaScriptCore
0 regressions found.
0 tests fixed.
OK.

Thanks!

Regards,
Chao-ying

[WebKit Review Bot]

Attachment 47887 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
orrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/Platform.h:109:  More than one command on the same line in if  [whitespace/parens] [4]
JavaScriptCore/jit/JITStubs.cpp:650:  Extra space before ( in function call  [whitespace/parens] [4]
JavaScriptCore/jit/JITStubs.cpp:689:  Extra space before ( in function call  [whitespace/parens] [4]
JavaScriptCore/jit/JITStubs.cpp:718:  Extra space before ( in function call  [whitespace/parens] [4]
JavaScriptCore/jit/JITStubs.cpp:1149:  Extra space before ( in function call  [whitespace/parens] [4]
JavaScriptCore/jit/JITStubs.cpp:1175:  Extra space before ( in function call  [whitespace/parens] [4]
JavaScriptCore/assembler/MacroAssembler.h:43:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:239:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:259:  shift_amount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:271:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:297:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:314:  shift_amount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:693:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:708:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:736:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:804:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/jit/JITOpcodes.cpp:1776:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/jit/ExecutableAllocator.h:202:  line_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/jit/ExecutableAllocator.h:203:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 57


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chao-ying Fu]

Hi,

  Is there anything we can help to make this patch committed?
Thanks a lot!

Regards,
Chao-ying

[Gabor Loki]

Some style issues... (please check the style bot)

Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. For example:

>+        ASSERT(*(insn - 1) == 0 && *(insn - 2) == 0 && *(insn - 3) == 0 && *(insn - 5) == 0);

or

>+        if (!imm.m_isPointer && imm.m_value == 0 && !m_fixedWidth)


>+    void lshift32(RegisterID shift_amount, RegisterID dest)
>+    {
>+        m_assembler.sllv(dest, dest, shift_amount);
>+    }

Do not use '_' in your variable names!

Some other issues...

Well, it would be nice if you define a new macro which can hold the instruction set architecture value and an other one which can test the requested instruction set. For example.:
#define WTF_MIPS_ARCH __mips
#define WTF_MIPS_ISA(v) (defined WTF_MIPS_ARCH && WTF_MIPS_ARCH == v)
or
#define WTF_MIPS_ISA_AT_LEAST(v) (defined WTF_MIPS_ARCH && WTF_MIPS_ARCH >= v)

Same for _ABIO32, __mips_isa_rev and others.

>+        __clear_cache(reinterpret_cast<char*>(start), reinterpret_cast<char*>(end));
>+#else
>+        intptr_t end = reinterpret_cast<intptr_t>(code) + size;
>+        __clear_cache(reinterpret_cast<char*>(code), reinterpret_cast<char*>(end));
>+#endif

Please read my comment about __clear_cache issue at https://bugs.webkit.org/show_bug.cgi?id=28886#c35 .
(Try to avoid internal __clear_cache function)


>+#if defined(__MIPSEB__)
Use CPU(BIG_ENDIAN)!

Anyway, this patch looks good to me as well ;)

BTW, do you have some comparison data about how the JIT performs on MIPS?

[Chao-ying Fu]

(In reply to comment #42)
> Some style issues... (please check the style bot)
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done
> without equality comparisons. For example:
> 
> >+        ASSERT(*(insn - 1) == 0 && *(insn - 2) == 0 && *(insn - 3) == 0 && *(insn - 5) == 0);
> 
> or
> 
> >+        if (!imm.m_isPointer && imm.m_value == 0 && !m_fixedWidth)
> 
> 
> >+    void lshift32(RegisterID shift_amount, RegisterID dest)
> >+    {
> >+        m_assembler.sllv(dest, dest, shift_amount);
> >+    }
> 
> Do not use '_' in your variable names!

  Yes.  I will fix all style issues that come from my code.

> 
> Some other issues...
> 
> Well, it would be nice if you define a new macro which can hold the instruction
> set architecture value and an other one which can test the requested
> instruction set. For example.:
> #define WTF_MIPS_ARCH __mips
> #define WTF_MIPS_ISA(v) (defined WTF_MIPS_ARCH && WTF_MIPS_ARCH == v)
> or
> #define WTF_MIPS_ISA_AT_LEAST(v) (defined WTF_MIPS_ARCH && WTF_MIPS_ARCH >= v)
> 
> Same for _ABIO32, __mips_isa_rev and others.

  Yes.  I will use WTF_* for all tests.  For _ABIO32, I will use it only in Platform.h and not in other .cpp/.h files, since I can only support O32 now.  In the future, if n32/o64 systems are added, we will define a new WTF_* for ABI selections.

> 
> >+        __clear_cache(reinterpret_cast<char*>(start), reinterpret_cast<char*>(end));
> >+#else
> >+        intptr_t end = reinterpret_cast<intptr_t>(code) + size;
> >+        __clear_cache(reinterpret_cast<char*>(code), reinterpret_cast<char*>(end));
> >+#endif
> 
> Please read my comment about __clear_cache issue at
> https://bugs.webkit.org/show_bug.cgi?id=28886#c35 .
> (Try to avoid internal __clear_cache function)

  I will change __clear_cache to __builtin____clear_cache that is a GCC builtin function from GCC manual.  Is it ok?

> 
> 
> >+#if defined(__MIPSEB__)
> Use CPU(BIG_ENDIAN)!

  Yes.

> 
> Anyway, this patch looks good to me as well ;)

  Thanks a lot!  I am testing my new patch and will post soon.
> 
> BTW, do you have some comparison data about how the JIT performs on MIPS?

  From my last December results, the non-jit SunSpider is 18774.6ms and the jit SunSpider is 10748.6ms on a MIPS 74kf 660Mhz sigma board.  The total time reduction is about 42%.  Thanks!

[Gabor Loki]

>   I will change __clear_cache to __builtin____clear_cache that is a GCC builtin
> function from GCC manual.  Is it ok?

Well, I don't mind using __clear_cache (or __builtin___clear_cache) on MIPS, but I have to note this is a *builtin* function. GCC community makes no promises about it. Very likely __clear_cache will be private in a later version of GCC (that was what happened in g++ 4.1.1 on ARM).

Just read the first section of http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html !

If you really like to use it, please guard __clear_cache with some GCC version checks.

[Chao-ying Fu]

(In reply to comment #44)
> >   I will change __clear_cache to __builtin____clear_cache that is a GCC builtin
> > function from GCC manual.  Is it ok?
> 
> Well, I don't mind using __clear_cache (or __builtin___clear_cache) on MIPS,
> but I have to note this is a *builtin* function. GCC community makes no
> promises about it. Very likely __clear_cache will be private in a later version
> of GCC (that was what happened in g++ 4.1.1 on ARM).
> 
> Just read the first section of
> http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html !
> 
> If you really like to use it, please guard __clear_cache with some GCC version
> checks.

  Yes, I will guard with GCC version check.
Note: I think __builtin____clear_cache() is a little bit better than __clear_cache(), since GCC will direct the call to __clear_cache() (that is implemented on MIPS) or to a system function.
And __clear_cache() can generate RTL instructions for better scheduling, compared to using asm() macros in c/cpp files.

  For MIPS Linux, we will call _flush_cache(), if GCC is not used.
Thanks!

Regards,
Chao-ying

[Chao-ying Fu]

Created attachment 49551 [details]
MIPS JIT Patch 20100225

Hi,

  This is the revised patch based on the review #42.
To get rid of some assembler warnings and a bug in the debug build,
I changed few MIPS asm() instructions for MIPS PIC code in JITStubs.cpp.

  The testing of release and debug build is ok.  Thanks!

Regards,
Chao-ying

[WebKit Review Bot]

Attachment 49551 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/Platform.h:109:  More than one command on the same line in if  [whitespace/parens] [4]
JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/jit/JITStubs.cpp:651:  Extra space before ( in function call  [whitespace/parens] [4]
JavaScriptCore/jit/JITStubs.cpp:690:  Extra space before ( in function call  [whitespace/parens] [4]
JavaScriptCore/jit/JITStubs.cpp:721:  Extra space before ( in function call  [whitespace/parens] [4]
JavaScriptCore/jit/JITStubs.cpp:1154:  Extra space before ( in function call  [whitespace/parens] [4]
JavaScriptCore/jit/JITStubs.cpp:1184:  Extra space before ( in function call  [whitespace/parens] [4]
JavaScriptCore/assembler/MacroAssembler.h:43:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/assembler/MacroAssemblerMIPS.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/jit/JITOpcodes.cpp:1787:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/jit/ExecutableAllocator.h:203:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 11 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Comment on attachment 49551 [details]
MIPS JIT Patch 20100225

Hi Chao-ying,

Many apologies, I thought I'd added a review last week, I guess I must have failed to hit commit.
Assuming you've fixed Gabor's review issues I'm happy, these changes look good to land. However, as raised in my previous review, I'll like to see this landed in two tranches.

You should be able to land yarr support with just the changes in assembler, yarr, jit/ExecutableAllocator.h, and some of the changes to wtf/Platform.h.

Please put up a patch with just these changes & I'll be happy to r+.

cheers,
G.

[Chao-ying Fu]

(In reply to comment #48)
> (From update of attachment 49551 [details])
> Hi Chao-ying,
> 
> Many apologies, I thought I'd added a review last week, I guess I must have
> failed to hit commit.
> Assuming you've fixed Gabor's review issues I'm happy, these changes look good
> to land. However, as raised in my previous review, I'll like to see this landed
> in two tranches.
> 
> You should be able to land yarr support with just the changes in assembler,
> yarr, jit/ExecutableAllocator.h, and some of the changes to wtf/Platform.h.
> 
> Please put up a patch with just these changes & I'll be happy to r+.
> 
> cheers,
> G.

Hi Gavin,

  Thanks!  I will post a yarr_jit patch soon.

Regards,
Chao-ying

[Chao-ying Fu]

Created attachment 49826 [details]
MIPS YARR JIT Patch 20100302

Here is the MIPS YARR JIT patch.  Thanks!

[Chao-ying Fu]

Hi All,

  Can someone help me to commit this patch?  Thanks a lot!

Regards,
Chao-ying

[Kenneth Rohde Christiansen]

You can commit it thought the commit-queue. Such as you set r? for requesting review you can set commit-queue? for requesting commit though it.

[Oliver Hunt]

Comment on attachment 49826 [details]
MIPS YARR JIT Patch 20100302

cq+

[WebKit Commit Bot]

Comment on attachment 49826 [details]
MIPS YARR JIT Patch 20100302

Clearing flags on attachment: 49826

Committed r55500: <http://trac.webkit.org/changeset/55500>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

reopening as we only landed the YARR JIT

[Chao-ying Fu]

Created attachment 50070 [details]
MIPS JIT Part2 Patch 20100304

Here is the part2 of the MIPS JIT patch.  Thanks a lot for help!

Regards,
Chao-ying

[Gabor Loki]

> Created an attachment (id=50070) [details]

The branch8 and branchTest8 functions are introduced at r55684.

Chao-ying, please add those functions to your last patch!

[Chao-ying Fu]

Created attachment 50335 [details]
MIPS JIT Part2 Patch 20100309

Here is the updated patch with branch8, branchTest8, and setTest8.
Thanks!

Regards,
Chao-ying

[Chao-ying Fu]

Created attachment 50862 [details]
MIPS JIT Part2 Patch 20100316

  I updated the patch (part2) for an issue in MIPS linkWithOffset().
The original function didn't support the conversion from bc1f to bc1t
when the target is too far.  This patch adds this conversion.
The test results are ok.  Thanks!

Regards,
Chao-ying

[Oliver Hunt]

Comment on attachment 50862 [details]
MIPS JIT Part2 Patch 20100316

Okay, I can't see anything wrong with this patch, i'll just talk to Gavin to see if he wants to look at it.

[Chao-ying Fu]

Hi Oliver,

  I need to ask one question about branch8.
Does branch8 assume that the value of "Imm32 right" is unsigned? 
Ex:
Jump branch8(Condition cond, Address left, Imm32 right)
{
  ASSERT(!(right.m_value & 0xFFFFFF00)); // ???
}

  If yes, I need to use lbu (unsigned load byte) for MIPS to have correct comparison.  Thanks a lot!

Regards,
Chao-ying

[Oliver Hunt]

(In reply to comment #62)
> Hi Oliver,
> 
>   I need to ask one question about branch8.
> Does branch8 assume that the value of "Imm32 right" is unsigned? 
> Ex:
> Jump branch8(Condition cond, Address left, Imm32 right)
> {
>   ASSERT(!(right.m_value & 0xFFFFFF00)); // ???
> }
> 
>   If yes, I need to use lbu (unsigned load byte) for MIPS to have correct
> comparison.  Thanks a lot!
> 
> Regards,
> Chao-ying

It should be an unsigned value :D

[Chao-ying Fu]

Created attachment 51972 [details]
MIPS JIT Part2 Patch 20100329

This new patch uses "lbu" to load unsigned byte.  Thanks!

[Oliver Hunt]

Comment on attachment 51972 [details]
MIPS JIT Part2 Patch 20100329

r=me!

[WebKit Commit Bot]

Comment on attachment 51972 [details]
MIPS JIT Part2 Patch 20100329

Rejecting patch 51972 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12580 test cases.
fast/loader/stateobjects/replacestate-then-pushstate.html -> failed

Exiting early after 1 failures. 7825 tests run.
141.60s total testing time

7824 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1601061

[Eric Seidel (no email)]

Comment on attachment 51972 [details]
MIPS JIT Part2 Patch 20100329

Looks like this was caused by bug 36647.  re-queueing.

[WebKit Commit Bot]

Comment on attachment 51972 [details]
MIPS JIT Part2 Patch 20100329

Clearing flags on attachment: 51972

Committed r56759: <http://trac.webkit.org/changeset/56759>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.