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

[bugzilla-daemon at webkit.org]

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


Gabor Loki <loki at inf.u-szeged.hu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |loki at inf.u-szeged.hu




--- Comment #5 from Gabor Loki <loki at inf.u-szeged.hu>  2009-10-07 02:00:04 PDT ---
> +    #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! :)

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