[webkit-reviews] review denied: [Bug 198102] [WASM-References] Support Anyref in globals : [Attachment 370381] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 198102: [WASM-References] Support Anyref in globals
https://bugs.webkit.org/show_bug.cgi?id=198102

Attachment 370381: Patch

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




--- Comment #2 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 370381
  --> https://bugs.webkit.org/attachment.cgi?id=370381
Patch

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

r- because concurrent GC issues in your asm. Will continue to review.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:533
> +	   append(block, Move, Arg::immPtr(tagCFunctionPtr<void*>(func,
B3CCallPtrTag)), callee);

👍🏼

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:566
> +    void emitWriteBarrierForJsWrapper();

"JsWrapper" => "JSWrapper"

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1075
> +    append(Move, Arg::bigImm(Instance::offsetOfOwner()), cell);

really big imm? I don't think that's actually needed. Just need to verify on
arm64. You could add an assert and just use this as an imm to the load.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1083
> +    append(Branch8, Arg::relCond(MacroAssembler::Above), Arg::addr(cell,
JSCell::cellStateOffset()), Arg::imm(blackThreshold));
> +    m_currentBlock->setSuccessors(continuation, slowPath);

This looks wrong since we're not guaranteed at a machine level that this load
happens before the store that happened above.

See SpeculativeJIT::compileStoreBarrier in the DFG.

AssemblyHelpers::barrierBranch() is loading the barrier threshold from the VM's
Heap itself. We don't have that here.