[Webkit-unassigned] [Bug 21611] Unnecessary operations, duplicated codes in the CodeGenerator

[bugzilla-daemon at webkit.org]

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





------- Comment #4 from ggaren at apple.com  2008-10-24 13:49 PDT -------
(From update of attachment 24360)
Looks good to me. I've asked Cameron to review further, since he wrote most of
the code being modified here.

Some simple style comments:

> +void ALWAYS_INLINE CodeGenerator::modifyLastBinaryOp(OpcodeID opcodeID, const int dst, const int src1, const int src2)
> +{
> +    ASSERT(instructions().size() >= 4);
> +    size_t size = instructions().size();
> +    instructions().at(size - 4).u.opcode = globalData()->machine->getOpcode(opcodeID);
> +    m_lastOpcodeID = opcodeID; 
> +    instructions().at(size - 3).u.operand = dst;
> +    instructions().at(size - 2).u.operand = src1;
> +    instructions().at(size - 1).u.operand = src2;
> +}

I would call this function "overwriteLastBinaryOp" instead of "modify", since
"modify" is more abstract.

I would add an ASSERT:
"ASSERT(globalData()->machine->isOpcode(instructions().at(size -
4).u.opcode))", to verify that the last instruction had the format you
expected.

> +void ALWAYS_INLINE CodeGenerator::modifyLastUnaryOp(OpcodeID opcodeID, const int dst, const int src)
> +{
> +    ASSERT(instructions().size() >= 3);
> +    size_t size = instructions().size();
> +    instructions().at(size - 3).u.opcode = globalData()->machine->getOpcode(opcodeID);
> +    m_lastOpcodeID = opcodeID; 
> +    instructions().at(size - 2).u.operand = dst;
> +    instructions().at(size - 1).u.operand = src;
> +}

Same comments here.

> +        static int unknown() {
> +            return OperandTypes().toInt();
> +        }

> 695  RegisterID* CodeGenerator::emitBinaryOp(OpcodeID opcode, RegisterID* dst, RegisterID* src1, RegisterID* src2, OperandTypes types)
>  694 RegisterID* CodeGenerator::emitBinaryOp(OpcodeID opcode, RegisterID* dst, RegisterID* src1, RegisterID* src2, int types)

I don't understand why you had to make these changes.

Annotating your ChangeLog with explanations of what you changed and why would
help explain this kind of thing to a reviewer, and a programmer looking back on
this code later. It's also the WebKit policy to do so.


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