[webkit-reviews] review granted: [Bug 226375] Put the Baseline JIT prologue and op_loop_hint code in JIT thunks. : [Attachment 430746] proposed patch.

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Mark Lam
<mark.lam at apple.com>'s request for review:
Bug 226375: Put the Baseline JIT prologue and op_loop_hint code in JIT thunks.
https://bugs.webkit.org/show_bug.cgi?id=226375

Attachment 430746: proposed patch.

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




--- Comment #18 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 430746
  --> https://bugs.webkit.org/attachment.cgi?id=430746
proposed patch.

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

r=me with some nits.

> Source/JavaScriptCore/ChangeLog:45
> +	   LinkBuffer size results:

Are these number from a specific workload?

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2313
> +    Jump branchAdd32(ResultCondition cond, TrustedImm32 imm, ImplicitAddress
destAddress)

I don't know anything about MIPS so I'm just gonna assume this is correct if
tests pass.

> Source/JavaScriptCore/jit/JIT.cpp:799
> +	       unsigned startArgument = m_codeBlock->isConstructor() ? 1 : 0;

Nit, I think you don't need the ternary here. True implicitly converts to
unsigned 1.

> Source/JavaScriptCore/jit/JIT.cpp:834
> +    unsigned generatorSelector = doesProfiling << 2 | isConstructor << 1 |
hasHugeFrame << 0;

Nit: Maybe this should be an inline function next to the macro for future code
changes?

> Source/JavaScriptCore/jit/JIT.cpp:898
> +	   static constexpr ThunkGenerator generators[] = {
> +#define USE_PROLOGUE_GENERATOR(doesProfiling, isConstructor, hasHugeFrame,
name, arityFixupName) arityFixupName,
> +	       FOR_EACH_PROLOGUE_GENERATOR(USE_PROLOGUE_GENERATOR)
> +#undef USE_PROLOGUE_GENERATOR
> +	   };

How does this not conflict with the generators definition on line 825? O.o

> Source/JavaScriptCore/jit/JIT.cpp:943
> +    store64(codeBlockGPR, addressFor(CallFrameSlot::codeBlock));

Nit: I think this should be storePtr.

> Source/JavaScriptCore/jit/JIT.cpp:966
> +    // We'll be imminently returning with a retab in the normal path, which
will do validation.

Nit: retab => retag.

> Source/JavaScriptCore/jit/JIT.cpp:1067
> +#endif

Maybe add an else with a #error "unsupported architecture" for each of these?

> Source/JavaScriptCore/jit/JIT.cpp:1069
> +    store64(codeBlockGPR, addressFor(CallFrameSlot::codeBlock));

Ditto on storePtr.