Bug 236367

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

Created attachment 451382 [details] RFC patch

(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.

Comment on attachment 451382 [details] RFC patch r- for now since EWS are red.

Created attachment 451893 [details] RFC patch Fixes Clang build errors.

Created attachment 451898 [details] RFC patch Fixes Xcode errors.

Created attachment 451899 [details] RFC patch Marks the header as private in Xcode, maybe that helps.

Created attachment 451906 [details] RFC patch With even more fixes.

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.

<rdar://problem/89026012>

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.

Created attachment 454079 [details] RFC patch - alternative Should now build via xcode.

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

(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.

(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.

(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.

(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.

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.

Created attachment 457918 [details] EWS run.

Created attachment 457967 [details] Patch

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

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.

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.

Created attachment 457973 [details] Patch for landing

Comment on attachment 457973 [details] Patch for landing Clearing flags on attachment: 457973 Committed r293092 (249803@trunk): <https://commits.webkit.org/249803@trunk>

All reviewed patches have been landed. Closing bug.