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

[bugzilla-daemon at webkit.org]

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





--- Comment #78 from Patrick R. Gansterer <paroga at paroga.com>  2011-03-01 15:22:37 PST ---
(From update of attachment 84214)
View in context: https://bugs.webkit.org/attachment.cgi?id=84214&action=review

I only looked at the style of this patch and added some comments, but in general:
1) Use 4 space indentation everywhere
2) Use early return
3) Remove the unneeded empty lines
4) Please use singe line comments: Two slashes followed by a space. Comment should be a full sentence with a "." at the end.

Running check-webkit-style will show you many of the style errors.

> Source/JavaScriptCore/ChangeLog:8
> +        Add YARR support for SH4 platforms (disabled by default).

Please remove the function names from ChangeLog if you don't add anything.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.cpp:3
> +/*
> + * Copyright (C) 2011 STMicroelectronics. All rights reserved.
> +*/

Please add a full header with LGPL or BDS license.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.cpp:60
> +}
> +} // namespace JSC

Missing empty line

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:330
> +      if ((srcDest != SH4Registers::r0) || (imm.m_value > 255) || (imm.m_value < 0)) {
> +          RegisterID scr = claimScratch();
> +          m_assembler.loadConstant((imm.m_value), scr);
> +          m_assembler.xorl_rr(scr, srcDest);
> +          releaseScratch(scr);
> +      } else
> +          m_assembler.xorl_i8r(imm.m_value, srcDest);

Please use 4 space intention.

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

Please remove this empty line.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:684
> +        return branchTrue();
> +

Please remove this empty line. Or move it before the return.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:745
> +        if (left != dest) {
> +            m_assembler.loadConstant(right.m_value, dest);
> +            set32Compare32(cond, left, dest, dest);
> +        } else {
> +            RegisterID scr = claimScratch();
> +            m_assembler.loadConstant(right.m_value, scr);
> +            set32Compare32(cond, left, scr, dest);
> +            releaseScratch(scr);
> +        }

We usually prefer early return.

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

No comment for this "section"?

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:858
> +            m_assembler.movt(dest);

Early return.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:871
> +           move(right, dest);
> +           set32Compare32(cond, left, dest, dest);

Early return.

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:892
> +            m_assembler.movt(dest);

Early return.

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

Empty line

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1256
> +        if ((cond == Zero) || (cond == NonZero)) {

Early return

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1413
> +    private:

No indentation

> Source/JavaScriptCore/assembler/MacroAssemblerSH4.h:1422
> +    static void linkCall(void* code, Call call, FunctionPtr function);
> +
> +    static void repatchCall(CodeLocationCall call, CodeLocationLabel destination);
> +
> +    static void repatchCall(CodeLocationCall call, FunctionPtr destination);
> +

No need for empty lines

> Source/JavaScriptCore/assembler/SH4Assembler.cpp:4
> +/*
> + *  SH4Assembler.cpp
> + *  Copyright (C) 2011 STMicroelectronics. All rights reserved.
> +*/

License header!

> Source/JavaScriptCore/assembler/SH4Assembler.h:50
> +    return ((opc) | (((rm) & 0xf) << 8) |(((rn) & 0xf) << 4));

Please use correct spaceing and remove the unneeded braces.
For the other functions in this file too!

> Source/JavaScriptCore/assembler/SH4Assembler.h:438
> +         ASSERT((claimscratchReg != 0x3));
> +         RegisterID scratchReg = scratchReg1;
> +
> +         if (!(claimscratchReg & 0x1))
> +             claimscratchReg = (claimscratchReg | 0x1);
> +         else {
> +             claimscratchReg = (claimscratchReg | 0x2);
> +             scratchReg = scratchReg2;
> +         }
> +         return scratchReg;
> +     }
> +
> +     void releaseScratch(RegisterID scratchR) 
> +     {   
> +         if (scratchR ==  scratchReg1)
> +             claimscratchReg = (claimscratchReg & 0x2);
> +         else
> +             claimscratchReg = (claimscratchReg & 0x1);
> +     } 

4 space indentation

> Source/JavaScriptCore/assembler/SH4Assembler.h:477
> +           RegisterID scr = claimScratch(); 

4 space indentation

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

Empty line

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

Empty line

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

Empty line

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

Empty line

> Source/JavaScriptCore/assembler/SH4Assembler.h:882
> +            uint16_t opc = getOpcodeGroup5(TSTIMM_OPCODE, imm);
> +            oneShortOp(opc);

Early return

> Source/JavaScriptCore/assembler/SH4Assembler.h:2078
> +    private:
> +        SH4Buffer m_buffer;

No indentation

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