[webkit-reviews] review denied: [Bug 129519] Support caching of custom setters : [Attachment 225708] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 129519: Support caching of custom setters
https://bugs.webkit.org/show_bug.cgi?id=129519

Attachment 225708: Patch
https://bugs.webkit.org/attachment.cgi?id=225708&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225708&action=review


So close to done!  Needs just a bit of love though.

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:197
> -	   int8_t registersFlushed;
> +	   SpillRegistersMode spillMode : 8;

OMG yes!

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:137
> -    : JITByIdGenerator(codeBlock, codeOrigin, usedRegisters, base, value,
registersFlushed)
> -    , m_scratch(scratch)
> -    , m_ecmaMode(ecmaMode)
> -    , m_putKind(putKind)
> +	   : JITByIdGenerator(codeBlock, codeOrigin, usedRegisters, base,
value, spillMode)
> +	   , m_scratch(scratch)
> +	   , m_ecmaMode(ecmaMode)
> +	   , m_putKind(putKind)

Pretty sure the original indentation was correct.

> Source/JavaScriptCore/jit/Repatch.cpp:1151
> +    GPRReg scratchGPR1 = InvalidGPRReg;
> +    if (prototypeChain)
> +	   scratchGPR1 = allocator.allocateScratchGPR();
> +
> +    allocator.preserveReusedRegistersByPushing(stubJit);

You should get rid of the ScratchRegisterAllocator code and instead assert that
there is always a scratch register available directly from the TempRegisterSet,
like:

TempRegisterSet(stubInfo.patch.usedRegisters).getFreeGPR()

And then assert that you get back something other than InvalidGPRReg.  You're
guaranteed to have a free register because you flushed all registers, and you
flushed all registers because you're making a call.

> Source/JavaScriptCore/jit/Repatch.cpp:1185
> +    if (allocator.didReuseRegisters())
> +	   allocator.restoreReusedRegistersByPopping(stubJit);

This can die now that you're flushing.

> Source/JavaScriptCore/jit/Repatch.cpp:1275
> +	       if (normalizePrototypeChain(exec, baseCell) ==
InvalidPrototypeChain)

Pretty sure you should be calling normalizePrototypeChainForChainAccess().