[Webkit-unassigned] [Bug 221260] [JSC] Enable WasmLLInt on ARMv7

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=221260

--- Comment #16 from Geza Lore <glore at igalia.com> ---
Comment on attachment 458054
  --> https://bugs.webkit.org/attachment.cgi?id=458054
v8 - rebase/2

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

Thanks for taking a look. Answers are inline.

>> Source/JavaScriptCore/llint/WebAssembly.asm:34
>> +    const NumberOfWasmArgumentJSRs = 6
> 
> Maybe we can find a more neutral name for this?

While I agree with you that the name is not ideal, JSR just matches the JITs/C++ use of a JSValueRegs to represent the same abstraction. The abstraction is "a 64-bit value, which is either a single GPR on 64-bit platforms or a pair of GPRs on 32-bit platform". JSValueRegs is exactly this (and JSValueRegs is not really more than that). Ideally there would be a more explicit G64Reg or something without the JS-ness in the name, but at the same time I am not sure adding another abstraction that describes the same concept as a JSValueRegs would help with clarity right now. Personally I would just rename JSValueRegs as it's a pretty generic thing. I don't really have strong feelings about the name, so long as it's unambiguous, and I am open to suggestions.

>> Source/JavaScriptCore/llint/WebAssembly.asm:716
>> +    subi CalleeSaveSpaceAsVirtualRegisters + NumberOfWasmArguments, t2
> 
> I guess this is guaranteed by the spec?

Not sure what you mean by 'this' here. t2 is loaded as an 'i' (32-bit) one line above, and both CalleeSaveSpaceAsVirtualRegisters and NumberOfWasmArguments are small constants defined at the top of the file, all of which are specific to the implementation.

>> Source/JavaScriptCore/llint/WebAssembly.asm:806
>> +        loadd -offset -  8 - CalleeSaveSpaceAsVirtualRegisters * 8[cfr], fpr
> 
> nit: spacing

Will fix shortly

>> Source/JavaScriptCore/runtime/JSCJSValue.h:165
>> +    static constexpr uint32_t WasmTag =         0xfffffffa;
> 
> Hmm. This seems confusing, since it would be a huge mistake for a value of this kind to be exposed to JS. I'm not sure what would look better though

Agreed, at the same time, the JSVALUE64 version nearby does pretty much the same thing I believe. We do need to be able to unambiguously differentiate these from JS values though.

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1804
>> +#if ENABLE(WEBASSEMBLY_SIGNALING_MEMORY)
> 
> Does this need a compile guard?

The enum value MemoryMode::Signaling is currently compiled out under the same guard, so right now the guard is needed. See Saam's comments and my reply regarding the same a few comments ago. I'm happy to add the enum value back, but signaling memory is unlikely to be ever supported on 32-bit platforms as it requires mprotecting 4G+ memory which is a non-starter, so on 32-bit platforms we will still want these UNREACHABLE_ON_PLATFORM, which would be at least as many compile guards I think.

>> Source/JavaScriptCore/wasm/WasmIndexOrName.cpp:35
>> +    ASSERT(!(index & allTags));
> 
> Should this be a release assert? I'm not sure why this check is here, does it have something to do with dropping the name section? I couldn't find anything in the blame either. @msaboff might know

The implementation relies on the top 2 bits of the 64-bit m_indexName.index as tags to denote which union representation is active, so the assert is there to make sure those bits don't alias the passed index. It's debug ASSERT as it's likely a pretty major dev bug if you pass an index that's >= 1<<62, so this is just here to help you find the bug quicker (not I just added these two asserts to the 64-bit implementation, otherwise it should be identical to before.)

>> Source/JavaScriptCore/wasm/WasmIndexOrName.cpp:49
>> +#endif
> 
> Should we have a #else error?

In reality, there are only 2 ports JSVALUE64 and JSVALUE32_64, so people often don't bother with the #elif and use #else. I always add both just so I can grep through the platform differences if I need to (consider it my poor attempt at documentation).

>> Source/JavaScriptCore/wasm/WasmInstance.h:-110
>> -            m_cachedBoundsCheckingSize = memory()->boundsCheckingSize();
> 
> Why does boundsCheckingSize exist then?

memory()->boundsCheckingSize() is the size of the allocated backing store (it's a misnomer, there is a more aptly named mappedCapacity() in Wasm::MemoryHandle, which is exactly), while memory()->size() is the size of the memory as allocated in Wasm (which changes with memory.grow). In case of Signaling memory (64-bit platforms only), sometimes we allocate a larger Wasm memory backing store than the Wasm memory initial size, but mprotect the excess. For bounds checking Wasm loads/stores we need memory()->size(). As of why boundsCheckingSize(), I am not sure it needs to. It's only used to initialize the CagedPtr one line above, whether that is correct is another question (I'm less familiar with CagedPtr as we don't have a Gigacage on 32-bit platforms), but probably should at least use mappedCapacity, so I will go and fix that.

>> Source/JavaScriptCore/wasm/WasmOperations.cpp:471
>> +JSC_DEFINE_JIT_OPERATION(operationConvertToI64, int64_t, (CallFrame* callFrame, EncodedJSValue v))
> 
> Why is this better as an EncodedJSValue?

On some ABIs, struct types are passed/returned via pointers, even if a primitive type with the same size would be passed by value via registers, or have other intricacies when it gets to calling convention (see https://godbolt.org/z/qdrxEv435, and https://godbolt.org/z/8szrrEjPa). EncodedJSValue is just int64_t, so a primitive type, which makes code generation at call sites simpler and consistent on all platforms. For this reason of portability, all functions that are called from JIT code should only receive and return primitive types.

>> Source/JavaScriptCore/wasm/WasmParser.h:211
>> +    memcpy(&result, source() + m_offset, sizeof(uint32_t)); // src can be unaligned
> 
> Is this only an issue now because unaligned access doesn't work on ARMv7?

alignof(uint32_t) is 4 (or more generally, for primitive types alignof(type) == sizeof(type)), on most implementations (incl x86 and ARM64), so I think this was UB even before as 'souce() + m_offset' could address any byte. As you suggest, on ARMv7, only a subset of load and store instructions support unaligned accesses on ARMv7, and this used to yield the wrong kind with the reinterpret_cast due to alignof(uing32_t) > 1.

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:-80
>> -                static_assert(std::is_same_v<Wasm::Instance*, typename FunctionTraits<decltype(operationAllocateResultsArray)>::ArgumentType<1>>);
> 
> Don't we still need this?

Maybe I again don't understand what 'this' refers to, but isn't this static_assert there a few lines below on line 86?

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:219
>> +        if constexpr (maxFrameExtentForSlowPathCall)
> 
> How does this work? Is this because we might have leafs that don't emit stack overflow checks?

On some ABIs (ARMv7 in particular this time), some arguments of operationAllocateResultsArray that we call below are passed on the stack due to lack of sufficient number of argument registers. The wrapper generated by createJSToWasmWrapper (and one place where marshallJSResult is used in JS->Wasm IC) did not allocate space for these, so we allocate it here (otherwise would thrash some stack values we need), if needed. maxFrameExtentForSlowPathCall is just enough bytes to pass stack arguments to any slow path call. Probably could allocate this excess in the wrapper/JS->Wasm IC that uses marshallJSResult, like we do in JITed functions that call slow path functions, but it's not always needed.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220427/2d8c79ec/attachment-0001.htm>