Put write barriers in the right places in the baseline JIT

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

This is another step on the way to GenGC.

[Mark Hahnenberg]

Created attachment 219674 [details]
Patch

[Filip Pizlo]

Comment on attachment 219674 [details]
Patch

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

Looks good except for the enter thing.

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1025
> +
> +    addSlowCase(jump());

Why doesn't this just call slow_path_enter here, instead of jumping?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:805
>          emitInitRegister(virtualRegisterForLocal(j).offset());
> +
> +    addSlowCase(jump());

Ditto.

[Oliver Hunt]

Comment on attachment 219674 [details]
Patch

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?

[Mark Hahnenberg]

(In reply to comment #3)
> (From update of attachment 219674 [details])
> 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?
It's guaranteed to be the scope object that we're storing to. In the case of putGlobalProperty, I guess that scope object is the global object. I guess we could just do a write barrier on the global object constant.

Are you asking why we can't just do a direct store to the mark bit of the global object? Because that's not how write barriers work. But we can definitely use the write barrier version that takes a JSCell* rather than a GPRReg which improves the size of the write barrier.

> 
> > 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 actually don't need to store the stack pointer. That was leftover cruft :-(

> 
> 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?
How does that lead to alignment + 1? Are we already misaligned by +1 while we're executing in the baseline JIT?

[Mark Hahnenberg]

Created attachment 219682 [details]
Patch

[Filip Pizlo]

Comment on attachment 219682 [details]
Patch

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

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:919
> +    for (unsigned i = 0; i < GPRInfo::numberOfRegisters; ++i)
> +        storePtr(GPRInfo::toRegister(i), &buffer[i]);

You're saving way too many registers.

[Mark Hahnenberg]

Created attachment 219695 [details]
Patch

[Filip Pizlo]

Comment on attachment 219695 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 219695 [details]
Patch

Clearing flags on attachment: 219695

Committed r160878: <http://trac.webkit.org/changeset/160878>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.