[webkit-reviews] review granted: [Bug 187373] New bytecode format for JSC : [Attachment 353170] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Tadeu Zagallo <tzagallo at apple.com>'s
request for review:
Bug 187373: New bytecode format for JSC
https://bugs.webkit.org/show_bug.cgi?id=187373

Attachment 353170: Patch

https://bugs.webkit.org/attachment.cgi?id=353170&action=review




--- Comment #156 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 353170
  --> https://bugs.webkit.org/attachment.cgi?id=353170
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353170&action=review

r=me

I think that my feedback about using common types is the kind of refinement
that we can talk about after this lands.  I think that the way this works
currently is already really nice.

Go ahead and land whenenver you're ready!

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:5700
> +	       auto bytecode = currentInstruction->as<OpJngreater>();

Question about the OpJxxx types - why don't all conditional jumps just share
the same type, like OpJCompare or something?

Then, it would be a little easier to handle casting of these things.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:57
> +    emit_compareAndJump<OpJless>(currentInstruction, LessThan);

... it would mean, for example, that emit_compareAndJump doesn't need to be
templatized.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:541
> +	   emitRightShiftFastPath<OpRshift>(currentInstruction,
JITRightShiftGenerator::SignedShift);

Similar thing here - could OpRshift and OpUrshift be the same type?

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1804
> +	   prepareForRegularCall
> +    )

I think that our typical style for llint macro calls would put the ')' right
after 'prepareForRegularCall' rather than on its own line.