Bug 221260 – [JSC] Enable WasmLLInt on ARMv7

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Xan Lopez]

The first Wasm tier can now be compiled independently (see bug #220365), but it's still 64bit only. This bug will track the effort needed to make it also work on 32bit platforms (right now ARMv7 and MIPS).

[Radar WebKit Bug Importer]

<rdar://problem/74141184>

[Xan Lopez]

Created attachment 455082 [details]
v1

Initial patch for review. All tests are passing on our side, but let's see what EWS says. See the ChangeLog for some high level comments about design decisions and (current) limitations.

[Xan Lopez]

Created attachment 455084 [details]
v2

Fix style.

[Geza Lore]

Comment on attachment 455084 [details]
v2

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

> Source/JavaScriptCore/ChangeLog:24
> +        We have decided to use a pair of GPRs for every type of wasm
> +        values, even if they are 32bit (i32/f32), with the odd numbered
> +        GPR holding the most significant (or only) value. This makes the

Actually, 32-bit values are held in the even-numbered (lower numbered) GPR.

[Geza Lore]

Comment on attachment 455084 [details]
v2

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

> Source/JavaScriptCore/ChangeLog:24
> +        We have decided to use a pair of GPRs for every type of wasm
> +        values, even if they are 32bit (i32/f32), with the odd numbered
> +        GPR holding the most significant (or only) value. This makes the

Actually, 32-bit values are held in the even-numbered (lower numbered) GPR.

[Geza Lore]

Created attachment 456107 [details]
rebase

[Xan Lopez]

Created attachment 456911 [details]
v4

v4, rebase

[Saam Barati]

Comment on attachment 456911 [details]
v4

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

> Source/JavaScriptCore/interpreter/CalleeBits.h:47
> +        : m_ptr { reinterpret_cast<void*>(static_cast<uintptr_t>(value >> (PayloadOffset * CHAR_BIT))) }
> +        , m_tag { static_cast<uint32_t>(value >> (TagOffset * CHAR_BIT)) }

why not just decode the value and grab payload and tag?

> Source/JavaScriptCore/runtime/JSCJSValue.h:604
> +ALWAYS_INLINE JSValue jsUnboxedFloat(float f)

nit: Maybe "wasmUnboxedFloat"?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:928
> +            if (type == B3::Int32 || type == B3::Float)

no need for "type == B3::Float" here since we're a GPR.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:932
>              if (type == B3::Int32 || type == B3::Float)

no need for "type == B3::Int32" here since we're a FPR.

> Source/JavaScriptCore/wasm/WasmCalleeGroup.cpp:171
> +#if ENABLE(WEBASSEMBLY_SIGNALING_MEMORY)

Do we really need to compile out this enum value?

Maybe we can achieve the same perf by adding some helpers that take in a mode and check if it's signaling vs bounds checking, and when !WEBASSEMBLY_SIGNALING_MEMORY, we can always return true from "isBoundsChecking"

> Source/JavaScriptCore/wasm/WasmCallingConvention.cpp:60
> +        for (unsigned i = 0; i < numberOfArgumentJSRs; ++i)

let's put braces here, since that's WK style.

[Geza Lore]

Created attachment 457058 [details]
v5 - fix review comment and rebase

[Geza Lore]

Comment on attachment 456911 [details]
v4

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

Fixed all bar compiling out the Signaling enum value. Let me know if you prefer I changed that too.

>> Source/JavaScriptCore/interpreter/CalleeBits.h:47
>> +        , m_tag { static_cast<uint32_t>(value >> (TagOffset * CHAR_BIT)) }
> 
> why not just decode the value and grab payload and tag?

Not sure how that happened, but indeed can do that, so I did.

>> Source/JavaScriptCore/wasm/WasmCalleeGroup.cpp:171
>> +#if ENABLE(WEBASSEMBLY_SIGNALING_MEMORY)
> 
> Do we really need to compile out this enum value?
> 
> Maybe we can achieve the same perf by adding some helpers that take in a mode and check if it's signaling vs bounds checking, and when !WEBASSEMBLY_SIGNALING_MEMORY, we can always return true from "isBoundsChecking"

The purpose of compiling this out wasn't so much performance, but getting the compiler to tell me where I need to fix things up, I found it quite difficult without. Now that it's done we can add this back in, though could catch issues in future changes, I'm happy either way.

[Geza Lore]

Created attachment 457329 [details]
v6 - tweak run-jsc-stress-tests

[Geza Lore]

Created attachment 458052 [details]
v7 - rebase

[Geza Lore]

Created attachment 458054 [details]
v8 - rebase/2

[Justin Michaud]

Comment on attachment 458054 [details]
v8 - rebase/2

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

This is very exciting! I left a few drive-by questions/comments.

> Source/JavaScriptCore/llint/WebAssembly.asm:34
> +    const NumberOfWasmArgumentJSRs = 6

Maybe we can find a more neutral name for this?

> Source/JavaScriptCore/llint/WebAssembly.asm:716
> +    subi CalleeSaveSpaceAsVirtualRegisters + NumberOfWasmArguments, t2

I guess this is guaranteed by the spec?

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

nit: spacing

> 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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1804
> +#if ENABLE(WEBASSEMBLY_SIGNALING_MEMORY)

Does this need a compile guard?

> 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

> Source/JavaScriptCore/wasm/WasmIndexOrName.cpp:49
> +#endif

Should we have a #else error?

> Source/JavaScriptCore/wasm/WasmInstance.h:-110
> -            m_cachedBoundsCheckingSize = memory()->boundsCheckingSize();

Why does boundsCheckingSize exist then?

> 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?

> 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?

> 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?

[Justin Michaud]

Comment on attachment 458054 [details]
v8 - rebase/2

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

> Source/JavaScriptCore/wasm/WasmOperations.cpp:471
> +JSC_DEFINE_JIT_OPERATION(operationConvertToI64, int64_t, (CallFrame* callFrame, EncodedJSValue v))

Why is this better as an EncodedJSValue?

[Geza Lore]

Comment on attachment 458054 [details]
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.

[Geza Lore]

Created attachment 458442 [details]
v9 - review fixes and rebase

[Geza Lore]

Created attachment 459047 [details]
v10 - rebase + fix 64-bit atomics

[Geza Lore]

Created attachment 459536 [details]
v11 - rebase, remove ChangeLog diffs

[Geza Lore]

Created attachment 459756 [details]
v12 - rebase

[Geza Lore]

I'm ready to offer beer to the person who r+ this patch.

[Saam Barati]

Comment on attachment 459756 [details]
v12 - rebase

r=me

[Saam Barati]

(In reply to Geza Lore from comment #21)
> I'm ready to offer beer to the person who r+ this patch.

🍻

[EWS]

Committed r294934 (251045@main): <https://commits.webkit.org/251045@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459756 [details].

[Mark Lam]

This is being rolled out in https://github.com/WebKit/WebKit/pull/1127 because it breaks WASM on 64-bit platforms.

[Mark Lam]

Rolled out in:
Committed r294965 (251069@main): https://commits.webkit.org/251069@main
https://github.com/WebKit/WebKit/commit/188a9080510836a099273218f8f2cf5f304b7f61

[Geza Lore]

Created attachment 460061 [details]
v13 - ARM64E fix

[Geza Lore]

Comment on attachment 460061 [details]
v13 - ARM64E fix

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

> Source/JavaScriptCore/wasm/WasmInstance.h:113
>          if (m_memory != nullptr) {
> -            m_cachedMemory = CagedPtr<Gigacage::Primitive, void, tagCagedPtr>(memory()->memory(), memory()->boundsCheckingSize());
> -            m_cachedBoundsCheckingSize = memory()->boundsCheckingSize();
> +            // This is used by the non-signaling based bound checking, so use the actual size of the memory.
> +            m_cachedBoundsCheckingSize = memory()->size();
> +            m_cachedMemory = CagedPtr<Gigacage::Primitive, void, tagCagedPtr>(memory()->memory(), m_cachedBoundsCheckingSize);
>              ASSERT(memory()->memory() == cachedMemory());
>          }

This is the only diff from the original reverted patch. (Now we tag m_cachedMemory with the same key as we use for untagging it)

> Tools/Scripts/run-jsc-stress-tests:1772
> +        if $isFTLPlatform

Also fixed this typo

[Mark Lam]

Comment on attachment 460061 [details]
v13 - ARM64E fix

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

>> Source/JavaScriptCore/wasm/WasmInstance.h:113
>>          }
> 
> This is the only diff from the original reverted patch. (Now we tag m_cachedMemory with the same key as we use for untagging it)

OK, I've looked into the issue.  The root cause of this issue is because you changed m_cachedBoundsCheckingSize to be set to memory()->size() previously, bit did not tag m_cachedMemory with the same.  Your current fix changes m_cachedMemory to be tagged with memory()->size() also.

However, looking through the code, I see that m_cachedBoundsCheckingSize is only used for this untagging m_cachedMemory.  Why did you change it to memory()->size() in the first place?  Can you not leave it as it was originally? 

In this patch, you also renamed boundsCheckingSize() to mappedCapacity(), and mappedCapacity() isn't used anywhere currently.  I can see how the name boundsCheckingSize() can be misleading if we don't use it for bounds checking in the non-signaling case.  However, I think it's appropriate to use mappedCapacity() as the tag value here instead of memory()->size().  mappedCapacity() never changes, but memory()->size() can change.  Let's stick with using mappedCapacity() because we want the tag to be "constant"-ish.  Can you change this code back to the way it was before and use mappedCapacity() as the tag value instead?

[Mark Lam]

Comment on attachment 460061 [details]
v13 - ARM64E fix

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

r=me

>>> Source/JavaScriptCore/wasm/WasmInstance.h:113
>>>          }
>> 
>> This is the only diff from the original reverted patch. (Now we tag m_cachedMemory with the same key as we use for untagging it)
> 
> OK, I've looked into the issue.  The root cause of this issue is because you changed m_cachedBoundsCheckingSize to be set to memory()->size() previously, bit did not tag m_cachedMemory with the same.  Your current fix changes m_cachedMemory to be tagged with memory()->size() also.
> 
> However, looking through the code, I see that m_cachedBoundsCheckingSize is only used for this untagging m_cachedMemory.  Why did you change it to memory()->size() in the first place?  Can you not leave it as it was originally? 
> 
> In this patch, you also renamed boundsCheckingSize() to mappedCapacity(), and mappedCapacity() isn't used anywhere currently.  I can see how the name boundsCheckingSize() can be misleading if we don't use it for bounds checking in the non-signaling case.  However, I think it's appropriate to use mappedCapacity() as the tag value here instead of memory()->size().  mappedCapacity() never changes, but memory()->size() can change.  Let's stick with using mappedCapacity() because we want the tag to be "constant"-ish.  Can you change this code back to the way it was before and use mappedCapacity() as the tag value instead?

I withdraw my comment.  m_cachedBoundsCheckingSize is used in bounds checks (I missed this previously).  And this necessitates that it be set to memory()->size().  

I've verified that updateCachedMemory() is called after the 2 places where MemoryHandle::growToSize() is called.  So, I think this is sound.

[Yusuke Suzuki]

Comment on attachment 460061 [details]
v13 - ARM64E fix

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

>>>> Source/JavaScriptCore/wasm/WasmInstance.h:113
>>>>          }
>>> 
>>> This is the only diff from the original reverted patch. (Now we tag m_cachedMemory with the same key as we use for untagging it)
>> 
>> OK, I've looked into the issue.  The root cause of this issue is because you changed m_cachedBoundsCheckingSize to be set to memory()->size() previously, bit did not tag m_cachedMemory with the same.  Your current fix changes m_cachedMemory to be tagged with memory()->size() also.
>> 
>> However, looking through the code, I see that m_cachedBoundsCheckingSize is only used for this untagging m_cachedMemory.  Why did you change it to memory()->size() in the first place?  Can you not leave it as it was originally? 
>> 
>> In this patch, you also renamed boundsCheckingSize() to mappedCapacity(), and mappedCapacity() isn't used anywhere currently.  I can see how the name boundsCheckingSize() can be misleading if we don't use it for bounds checking in the non-signaling case.  However, I think it's appropriate to use mappedCapacity() as the tag value here instead of memory()->size().  mappedCapacity() never changes, but memory()->size() can change.  Let's stick with using mappedCapacity() because we want the tag to be "constant"-ish.  Can you change this code back to the way it was before and use mappedCapacity() as the tag value instead?
> 
> I withdraw my comment.  m_cachedBoundsCheckingSize is used in bounds checks (I missed this previously).  And this necessitates that it be set to memory()->size().  
> 
> I've verified that updateCachedMemory() is called after the 2 places where MemoryHandle::growToSize() is called.  So, I think this is sound.

Wait, this is not correct.
For shared wasm memory, we are *intentionally* using mapped-capacity because of growing feature from concurrently running wasm threads. Please read the commit message in https://github.com/WebKit/WebKit/commit/80581efa4a373a74da13227ba20e254b4efaa357

[Geza Lore]

Created attachment 460125 [details]
Patch

[Geza Lore]

Created attachment 460126 [details]
v14 - fix wasm shared memory

[Geza Lore]

Created attachment 460127 [details]
Diff from v13 to v14

[Geza Lore]

OK, I fixed the m_cachedBoundsCheckingSize

What I went with is the following:

1. On all platforms except CPU(ARM), m_cachedBoundsCheckingSize = memory()->mappedCapacity(), as we have done before, so no change on any of these platforms.

2. On ARMv7, as we don't have signal handlers yet, we cannot safely grow a shared memory in a multi-threaded environment (at least not as the code is right now), but I want to keep as many tests enabled as possible, so with CPU(ARM), we set m_cachedBoundsChackingSize = memory()->size(). This works for non-shared memories as for those mappedCapacity() and size() is always the same anyway. For shared memories, this makes them work with their initial size, and I added a release assert that fires when trying to grow a shared memory. With this, we only require disabling 3 tests on ARMv7, that try to do a grow operation on a shared memory.

In summary: on old platforms, everything is as it was. On ARMv7, wasm shared memories work with the limitation that they cannot be grown (i.e.: they are stuck at their initial capacity).

For convenience, I attached the diff between v13 (last reviewed) and v14 (with this change).

[Mark Lam]

Comment on attachment 460126 [details]
v14 - fix wasm shared memory

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

r=me with revert of unneeded change in LLIntOfflineAsmConfig.h.

> Source/JavaScriptCore/llint/LLIntOfflineAsmConfig.h:151
> +#if USE(JSVALUE32_64)
> +#define OFFLINE_ASM_JSVALUE32_64 1
> +#else
> +#define OFFLINE_ASM_JSVALUE32_64 0
> +#endif

Why is this needed?  Isn't this already inferred by the inverse of OFFLINE_ASM_JSVALUE64?  Your patch also doesn't use this.  Declaring a new OFFLINE_ASM_xxx can have a measurable impact on build times as it causes LLIntAssembly.h (or at least it used to the last time we measured this).  Let's not add a new one unless there's a good reason to.  Please remove this before landing.

[Geza Lore]

> Declaring a new OFFLINE_ASM_xxx can have a measurable impact on build times as it causes LLIntAssembly.h (or at least it used to the last time we measured this).

... to what?

[Geza Lore]

Created attachment 460142 [details]
v15 - revert unneded change in LLIntOfflineAsmConfig.h

[Mark Lam]

(In reply to Geza Lore from comment #37)
> > Declaring a new OFFLINE_ASM_xxx can have a measurable impact on build times as it causes LLIntAssembly.h (or at least it used to the last time we measured this).
> 
> ... to what?

Sorry.  I meant that it causes the permutations in LLIntAssembly.h to double.  We previously observed that this increased compilation time noticeably.

On second thought, I may be wrong about the additional declaration increasing the permutations.  If it's not used in the .asm files, then maybe it won't increase the permutations.  But every newly used OFFLINE_ASM configuration value like this will effectively double the permutations in LLIntAssembly.h.

Regardless, it's not good to have an unused configuration option and it is also already implied by JSVALUE64 (which is already defined).

[Geza Lore]

Thanks for the explanation. I do agree with the last sentence of course.

[EWS]

Committed r295449 (251455@main): <https://commits.webkit.org/251455@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 460142 [details].