[webkit-reviews] review denied: [Bug 125975] Put write barriers in the right places in the baseline JIT : [Attachment 219674] Patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 125975: Put write barriers in the right places in the baseline JIT
https://bugs.webkit.org/show_bug.cgi?id=125975

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

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219674&action=review


r- due to stack push/pop shenanigans vs. single add/sub

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:806
> +    emitLoad(value, regT3, regT2);
> +    
> +    emitWriteBarrier(regT0, regT3, regT1, regT2, ShouldFilterImmediates);

Are regT0/regT3 already guaranteed to be the global reference?	Also why can't
we just directly flag the global object mark bit?

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1036
> +    for (unsigned i = 0; i < GPRInfo::numberOfRegisters; ++i)
> +	   storePtr(GPRInfo::toRegister(i),
&vm()->writeBarrierRegisterBuffer[i]);
> +    push(scratch);
> +    push(scratch);
> +    push(scratch);
> +    storePtr(MacroAssembler::stackPointerRegister,
&vm()->writeBarrierRegisterBuffer[GPRInfo::numberOfRegisters]);
> +    andPtr(TrustedImm32(~0xf), MacroAssembler::stackPointerRegister);
> +
> +    callOperation(operationUnconditionalWriteBarrier, owner);
> +
> +    loadPtr(&vm()->writeBarrierRegisterBuffer[GPRInfo::numberOfRegisters],
MacroAssembler::stackPointerRegister);
> +    pop(scratch);
> +    pop(scratch);
> +    pop(scratch);
> +    for (unsigned i = 0; i < GPRInfo::numberOfRegisters; ++i)
> +	   loadPtr(&vm()->writeBarrierRegisterBuffer[i],
GPRInfo::toRegister(i)); 
> +
> +    ownerNotMarked.link(this);

Why isn't this just a single stack decrement and then frame relative
load/store?  Among other things it would drop your need to store the stack
pointer into a side slot.

I'm also unconvinced you're going to get correct alignment - doesn't the call
push the return address on x86? that leads to alignment+1 in the callee - or is
that what the callee expects?