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

[bugzilla-daemon at webkit.org]

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


Gavin Barraclough <barraclough at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #42679|                            |review-
               Flag|                            |




--- Comment #31 from Gavin Barraclough <barraclough at apple.com>  2009-11-13 16:32:49 PST ---
(From update of attachment 42679)
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.

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