Bug 236367 – [JSC] Accommodate the RISCV64 C calling convention

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

The RISCV64 C calling convention specifies that 32-bit types have to be sign-extended into the upper 32 bits of the 64-bit integer registers. This goes for both signed and unsigned values. Behavior of 8-bit or 16-bit types depends on signedness but it still extends to the whole size of the 64-bit integer register -- unsigned values are zero-extended, signed values are sign-extended.

This is causing problems with how the current JIT-generated code is passing argument values to native JIT functions and how those are then used. JIT generation right now assumes that a 32-bit value can be packed either directly into the lower 32 bits of the argument register or through the narrower 32-bit aliases (e.g. w0-w7 registers on ARM64, edi/esi on x86_64). In the native function, these 32-bits-wide registers are then used directly in different operations on these values.

RV64 has it more complicated. Any native operation on registers containing values of bit-width 32 or smaller expects those values to be properly extended into the upper bits. Right now, this isn't guaranteed and causes problems when the 32-bit value has 1 in bit#31 that isn't extended into the upper 32 bits of the 64-bit integer register.

[Zan Dobersek]

Calling convention specification:
https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf

[Zan Dobersek]

Created attachment 451382 [details]
RFC patch

[Zan Dobersek]

(In reply to Zan Dobersek from comment #2)
> Created attachment 451382 [details]
> RFC patch

This patch introduces the CCallInteger template through which values of the desired integer type would be accessed in the native JIT operations. The default implementation provides a simple implicit conversion operator that retrieves the value, with an additional conversion operator for converting to enumerator values. The implicit conversion operator minimizes the amount of needed changes in existing code, except for a few now-necessary explicit casts.

For RISCV64, a separate implementation of CCallInteger is provided, with same interface but an implicit conversion operator that on each invocation returns the sign-extended underlying value. This approach avoids any changes in the JIT generation but instead imposes alignment with the calling convention requirements at the point of use.

Main downside is the sign extension that now occurs on each value access, meaning one or two additional instructions for each such occasion. There's also missing sign extension for 8-bit or 16-bit values but those are trivial to add.

The CCallInteger template is then aliased to different descriptive sub-types (like CCallInt32, CCallUIntptr) and the declarations and definitions of all native JIT operations are adjusted to use these types for parameters and return values. CCallHelpers::setupArgumentsImpl() overloads are changed so that immediate values only fit into CCallInteger<> parameters in the target operations.

This is just the initial idea, so any feedback is welcome. It avoids changing JIT details, but it imposes a lot of changes in signatures for the native JIT operations, but even then it shouldn't change behavior for non-RISCV64 platforms.

[Yusuke Suzuki]

Comment on attachment 451382 [details]
RFC patch

r- for now since EWS are red.

[Zan Dobersek]

Created attachment 451893 [details]
RFC patch

Fixes Clang build errors.

[Zan Dobersek]

Created attachment 451898 [details]
RFC patch

Fixes Xcode errors.

[Zan Dobersek]

Created attachment 451899 [details]
RFC patch

Marks the header as private in Xcode, maybe that helps.

[Zan Dobersek]

Created attachment 451906 [details]
RFC patch

With even more fixes.

[Zan Dobersek]

Comment on attachment 451906 [details]
RFC patch

This now builds everywhere, but it's not yet reviewable. The desire is to gather feedback on the approach, hence the RFC call.

[Radar WebKit Bug Importer]

<rdar://problem/89026012>

[Zan Dobersek]

Created attachment 453981 [details]
RFC patch - alternative

This is an alternative approach that reduces the amount of changes but is more lenient and puts more burden to the person writing out the definitions of JIT operations.

There's no change to argument marshalling in CCallHelpers.h, or the integer types for JIT operations. Only in the definitions would the integer arguments have to use macros to specify the type and the name of the argument. In the definition body, another macro would be used to finally "define" the integer argument, assigning the argument value to the desired name.

For non-RISCV64 platforms this assignment would be pass-through. For RISCV64, the necessary sign extension would be applied to the value.

[Zan Dobersek]

Created attachment 454079 [details]
RFC patch - alternative

Should now build via xcode.

[Saam Barati]

Comment on attachment 451906 [details]
RFC patch

Out of the two approaches, this one probably makes more sense. But I like that the second approach is smaller, but don't love that you need to change the implementation of the various operations. 

I wonder if it's possible to do some kind of crazy macro/template magic and change JSC_DEFINE_JIT_OPERATION to automatically do what we want? That way no macros applied to types of arguments or to implementation of the functions

[Zan Dobersek]

(In reply to Saam Barati from comment #13)
> I wonder if it's possible to do some kind of crazy macro/template magic and
> change JSC_DEFINE_JIT_OPERATION to automatically do what we want? That way
> no macros applied to types of arguments or to implementation of the functions

DEFINE_JIT_OPERATION reproduces the function signature, along with different attributes, and expects the function body afterwards. It would probably be possible to include the body in the macro invocation, and then for RISCV64 construct a function body that first sign-extends whatever is necessary before laying in the actual operation implementation.

This would also require tidy parameter lists that always consist of a type-and-name so that these are passed through separate macros extracting either the type or the name of the parameter.

So more faith put into the preprocessor. I don't think C++ templates would be usable directly in DEFINE_JIT_OPERATION since C linkage is required, and that's rejected for templates.

[Zan Dobersek]

(In reply to Zan Dobersek from comment #14)
> This would also require tidy parameter lists that always consist of a
> type-and-name so that these are passed through separate macros extracting
> either the type or the name of the parameter.
> 

Tried this, it's just problems and borderline preprocessor acrobatics of getting things in proper place. I'm skeptical a good solution is possible.

[Zan Dobersek]

(In reply to Saam Barati from comment #13)
> Comment on attachment 451906 [details]
> RFC patch
> 
> Out of the two approaches, this one probably makes more sense. But I like
> that the second approach is smaller, but don't love that you need to change
> the implementation of the various operations. 
> 

I like this approach more cause it gives the opportunity of also cleaning up and clamping down the exact sizes for each integer parameter. It's not the ideal solution cause for RISC-V it will end up doing sign extension on every conversion to the native integer type, but it's the most complete one, it scales into the future and there's no drawbacks for other ISAs.

[Zan Dobersek]

(In reply to Zan Dobersek from comment #16)
> (In reply to Saam Barati from comment #13)
> > Comment on attachment 451906 [details]
> > RFC patch
> > 
> > Out of the two approaches, this one probably makes more sense. But I like
> > that the second approach is smaller, but don't love that you need to change
> > the implementation of the various operations. 
> > 
> 
> I like this approach more cause it gives the opportunity of also cleaning up
> and clamping down the exact sizes for each integer parameter. It's not the
> ideal solution cause for RISC-V it will end up doing sign extension on every
> conversion to the native integer type, but it's the most complete one, it
> scales into the future and there's no drawbacks for other ISAs.

Not going to work out. The templated struct embedding the integer value has to be kept a POD type so that the function can continue using the necessary C linkage. It's doable but it requires a wide usage of aggregate initializers for these objects.

But the blocker is the Apple ARM64 calling convention which enables function arguments of small integer types to be placed on the stack using an alignment smaller than 8 bytes. It doesn't allow this for compact types, which is what the integer-containing POD struct is treated as. So this won't work as intended, and if it did, it might cause unwanted regressions or at least changes of behavior that might not be entirely desired.

[Zan Dobersek]

Created attachment 457865 [details]
WIP patch

This makes it much simpler, it leaves it to the CCallHelpers::setupArguments() call to, at the end of setup, run through the operation arguments, detecting where the sign extension is necessary.

This one should work only for the Baseline JIT, and only for entering CCalls. Higher levels will need separate attention, and e.g. test cases in testmasm and other test programs will require manual sign-extension of the returned 32-bit integer value.

This is still a prototyping patch, the behavior has to be limited to RISC-V, and maybe some additional validation should be added to ensure that only doubles are used as floating-point arguments for any given operation, meaning any other type uses GPRs.

[Zan Dobersek]

Created attachment 457918 [details]
EWS run.

[Zan Dobersek]

Created attachment 457967 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 457967 [details]
Patch

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

I think this patch looks good. r=me with comments.

> Source/JavaScriptCore/jit/CCallHelpers.h:691
> +        if (!std::is_same<ArgumentType, double>::value) {

Use `if constexpr`. And use is_same_v
Also, is it correct? what happens if we are using `float`?
Can you align it to the condition where CCallHelpers use FPRReg?

> Source/JavaScriptCore/jit/CCallHelpers.h:696
> +            if (isRISCV64() && gprIndex < GPRInfo::numberOfArgumentRegisters
> +                && std::is_integral<ArgumentType>::value && sizeof(ArgumentType) == 4) {

Use `if constexpr` and use is_integral_v

[Yusuke Suzuki]

Comment on attachment 457967 [details]
Patch

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

>> Source/JavaScriptCore/jit/CCallHelpers.h:691
>> +        if (!std::is_same<ArgumentType, double>::value) {
> 
> Use `if constexpr`. And use is_same_v
> Also, is it correct? what happens if we are using `float`?
> Can you align it to the condition where CCallHelpers use FPRReg?

Note that wasm has float.

[Zan Dobersek]

Comment on attachment 457967 [details]
Patch

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

>>> Source/JavaScriptCore/jit/CCallHelpers.h:691
>>> +        if (!std::is_same<ArgumentType, double>::value) {
>> 
>> Use `if constexpr`. And use is_same_v
>> Also, is it correct? what happens if we are using `float`?
>> Can you align it to the condition where CCallHelpers use FPRReg?
> 
> Note that wasm has float.

Everything going through CCallHelpers, if floating-point, is of the double type. There's ceilFloat and floorFloat that are used in B3 as fallbacks, but that doesn't affect CCallHelpers.

Before being marshalled, every incoming FPRReg is static-asserted to be going against a double-typed argument in any given operation:
https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/jit/CCallHelpers.h#L419

I'll add a comment explaining the situation.

>> Source/JavaScriptCore/jit/CCallHelpers.h:696
>> +                && std::is_integral<ArgumentType>::value && sizeof(ArgumentType) == 4) {
> 
> Use `if constexpr` and use is_integral_v

The whole function is constexpr already. I'lll remove ALWAYS_INLINE though cause it's meaningless.

[Zan Dobersek]

Created attachment 457973 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 457973 [details]
Patch for landing

Clearing flags on attachment: 457973

Committed r293092 (249803@trunk): <https://commits.webkit.org/249803@trunk>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.