[Webkit-unassigned] [Bug 30144] MIPS JIT Supports

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30144





--- Comment #10 from Chao-ying Fu <fu at mips.com>  2009-10-07 14:35:44 PDT ---
(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.  :-)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.