[Webkit-unassigned] [Bug 44329] SH4 JIT SUPPORT

[bugzilla-daemon at webkit.org]

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





--- Comment #80 from Patrick R. Gansterer <paroga at paroga.com>  2011-03-02 10:12:32 PST ---
(From update of attachment 84424)
View in context: https://bugs.webkit.org/attachment.cgi?id=84424&action=review

Style improved a lot, but still some issues. Most of them are valid for whole code.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.cpp:32
> +

No need for this empty line

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:31
> +#include "assert.h"
> +#include <wtf/Platform.h>

I don't think this headers are required.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:32
> +#if ENABLE(ASSEMBLER)

Use ENABLE(ASSEMBLER) && CPU(SH4)

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:42
> +

Empty line

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:363
> +    void load32(RegisterID base, int offset, RegisterID dest)

You can use early return in this function too.
There are many similar if/else constructs which should use early return.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:815
> +       m_assembler.loadConstant(imm.m_value, dest);

4 space indentation

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:822
> +       DataLabelPtr dataLabel(this);
> +       m_assembler.loadConstant(reinterpret_cast<uint32_t>(initialValue.m_value), dest, true);
> +       return dataLabel;

4 space indentation

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1424
> +    static void linkCall(void*, Call, FunctionPtr);
> +
> +    static void repatchCall(CodeLocationCall, CodeLocationLabel);
> +
> +    static void repatchCall(CodeLocationCall, FunctionPtr);

Empty lines

> Source/JavaScriptCore/assembler/SH4Assembler.cpp:36
> +#ifdef SH4_ASSEMBLER_TRACING
> +bool SH4Assembler::dumpNativeCode = false;
> +#endif

I don't see a ASSEMBLER_TRACING for the other cpus. So I'm not sure if we want this code at all.
If want the tracing code in the tree, I'd prefer to get rid of this variable and move all code into a bool dumpNativeCode() in the header.
Using a own file for this variable is overkill IMHO.

> Source/JavaScriptCore/assembler/SH4Assembler.h:30
> +#include <wtf/Platform.h>

IMHO no need for this header

> Source/JavaScriptCore/assembler/SH4Assembler.h:42
> +#ifndef va_list
> +#include <stdarg.h>
> +#endif

Can't we add stdarg.h all the time?

> Source/JavaScriptCore/assembler/SH4Assembler.h:48
> +inline uint16_t getOpcodeGroup1(uint16_t opc, int rm, int rn)

Why are these function not in the JSC namespace?

> Source/JavaScriptCore/assembler/SH4Assembler.h:365
> +        } Condition;

I don't think this enum needs so much indentation. ;-)

> Source/JavaScriptCore/assembler/SH4Assembler.h:649
> +        case 1: opc = getOpcodeGroup2(SHLL_OPCODE, dst);
> +            break;

Please move the getOpcodeGroup2 and ASSERT_NOT_REACHED calls into their own lines.

> Source/JavaScriptCore/assembler/SH4Assembler.h:951
> +        default: ASSERT_NOT_REACHED();

New line for ASSERT_NOT_REACHED

> Source/JavaScriptCore/assembler/SH4Assembler.h:1101
> +    // double operations

You usually have an empty line after the "section header"

> Source/JavaScriptCore/assembler/SH4Assembler.h:1154
> +             fmovReadr0r(base, dest);

Early return

> Source/JavaScriptCore/assembler/SH4Assembler.h:1229
> +    // Various move ops:

You don't use a colon at the other "section headers". Please add it to all other or remove it everywhere

> Source/JavaScriptCore/assembler/SH4Assembler.h:1373
> +

empty line

> Source/JavaScriptCore/assembler/SH4Assembler.h:1658
> +         }

4 space indentation

> Source/JavaScriptCore/assembler/SH4Assembler.h:1711
> +            *addr = offsetBits;

early return

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