Closed Bug 1455028 Opened 2 years ago Closed 2 years ago

[MIPS64] Fix jit -> c++ int32 argument passing for simulator builds

Categories

(Core :: JavaScript Engine: JIT, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: dragan.mladjenovic, Assigned: dragan.mladjenovic)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch bug1455028.patch (obsolete) — Splinter Review
While testing Bug 1455019 I discovered that changes I introduced in Bug 1453628 would produce incorrect result on simulator / c++ boundary for functions that have for example void* (*)(double) signature whilst their abi signature value being Args_Int_Double because Int in this context actually labels general purpose register. To make things simple this patch reverts changes done by previous one and does explicit result value coercion for known external function that requires it.
Assignee: nobody → dragan.mladjenovic
Attachment #8968983 - Flags: review?(bbouvier)
Duplicate of this bug: 1455027
Priority: -- → P5
Comment on attachment 8968983 [details] [diff] [review]
bug1455028.patch

Review of attachment 8968983 [details] [diff] [review]:
-----------------------------------------------------------------

It's weird. I think there should be a clear cutoff between General and Int in the Prototype_ typedefs family. In the example you mention, could it alternatively be solved by using the more precise return type, viz. General, when a function returns a pointer type, instead of Int?

Or are the explicit coercions needed because the C++ compiler doesn't clear the high 32 bits when returning int32 values?

(waiting on answers before r+)

::: js/src/jit/mips64/Simulator-mips64.cpp
@@ +2242,5 @@
>            case Args_Int_Double: {
>              double dval0 = getFpuRegisterDouble(12);
>              Prototype_Int_Double target = reinterpret_cast<Prototype_Int_Double>(external);
> +            int64_t result = target(dval0);
> +            if(external == intptr_t((int32_t(*)(double))JS::ToInt32)) {

nit: here and below: space before opening parenthesis, no need for {} for single line if-bodies

Can the explicit function type coercion be removed? Seems that a compiler might have all the information to convert directly to intptr_t, but, oh well..
(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Comment on attachment 8968983 [details] [diff] [review]
> bug1455028.patch
> 
> Review of attachment 8968983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's weird. I think there should be a clear cutoff between General and Int
> in the Prototype_ typedefs family. In the example you mention, could it
> alternatively be solved by using the more precise return type, viz. General,
> when a function returns a pointer type, instead of Int?
> 
> Or are the explicit coercions needed because the C++ compiler doesn't clear
> the high 32 bits when returning int32 values?
> 


Yes that is correct. The issue would be solved by using more precise return type. In fact compiler does clear high 32 bits (not sure if you can rely on it to do so), but we are expecting them to be sign extension of lower 32 bits. By looking into FunctionInfo source (https://searchfox.org/mozilla-central/search?q=symbol:T_js%3A%3Ajit%3A%3ATypeToDataType&redirect=false) it seems that that path of calling into C++ does not expect int32 as return value, so the only offenders would be wasm builtin calls(for now). The whole thing with AbiArgs could be cleaned up. At lest "Int" could be replaced with "General" to avoid further confusion. I'm not sure what is the scope of changes needed  to disambiguate between int32 return value and the pointer/boolean/.. ones, so I took the path of least resistance with this manual coercion.
Comment on attachment 8968983 [details] [diff] [review]
bug1455028.patch

Review of attachment 8968983 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, thanks for your answer. r=me with previous comments addressed. Thanks!
Attachment #8968983 - Flags: review?(bbouvier) → review+
Attachment #8968983 - Attachment is obsolete: true
Attachment #8970829 - Flags: review+
Keywords: checkin-needed
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64b83e067db2
[MIPS64] Fix jit -> c++ int32 argument passing for simulator builds; r=bbouvier
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/53422b422071
[MIPS64] Fix jit -> c++ int32 argument passing for simulator builds. r=bbouvier
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6e69352a940e
Backed out 3 changesets (bug 1455028, bug 1455019, bug 1455016) because they were already pushed to inbound. DONTBUILD
https://hg.mozilla.org/mozilla-central/rev/64b83e067db2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.