958672 - IonMonkey: Small cleanups to calling infrastructure after MPassArg removal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: 0001-Remove-argslot-from-MCall-LIR.patch

A couple of small changes while working in this area.

Part 1: Remove unnecessary |argslot| from MCall-derived LIR.

Comment 1

[Sean Stangl [:sstangl]]

Attachment: 0002-Don-t-load-unobservable-return-values.patch

Part 2: Don't load unobservable return values in LCallNative and LCallDOMNative.

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 8358582 [details] [diff] [review]
0001-Remove-argslot-from-MCall-LIR.patch

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

r+ with nit applied

::: js/src/jit/LIR-Common.h
@@ +1044,1 @@
>                   const LDefinition &nargsreg, const LDefinition &tmpobjreg)

Can you move the nargsreg to the line above?

@@ +1065,4 @@
>    public:
>      LIR_HEADER(CallKnown)
>  
> +    LCallKnown(const LAllocation &func, const LDefinition &tmpobjreg) {

For constructors we put the { on a newline in IonMonkey

::: js/src/jit/Lowering.cpp
@@ +414,5 @@
>          GetTempRegForIntArg(2, 0, &privReg);
>          mozilla::DebugOnly<bool> ok = GetTempRegForIntArg(3, 0, &argsReg);
>          MOZ_ASSERT(ok, "How can we not have four temp registers?");
> +        LCallDOMNative *lir = new(alloc()) LCallDOMNative(tempFixed(cxReg),
> +            tempFixed(objReg), tempFixed(privReg), tempFixed(argsReg));

Align the arguments like before. So adding spaces so it comes under the first argument of LCallDOMNative(.

@@ +432,5 @@
>              mozilla::DebugOnly<bool> ok = GetTempRegForIntArg(3, 0, &tmpReg);
>              MOZ_ASSERT(ok, "How can we not have four temp registers?");
>  
> +            LCallNative *lir = new(alloc()) LCallNative(tempFixed(cxReg),
> +                tempFixed(numReg), tempFixed(vpReg), tempFixed(tmpReg));

Align the arguments like before. So adding spaces so it comes under the first argument of LCallNative(.

@@ +443,5 @@
>      }
>  
>      // Call anything, using the most generic code.
>      LCallGeneric *lir = new(alloc()) LCallGeneric(useFixed(call->getFunction(), CallTempReg0),
> +        tempFixed(ArgumentsRectifierReg), tempFixed(CallTempReg2));

Align the arguments like before. So adding spaces so it comes under the first argument of LCallGeneric(.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8358584 [details] [diff] [review]
0002-Don-t-load-unobservable-return-values.patch

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

Nice. Does this happen in a known case?

Comment 4

[Sean Stangl [:sstangl]]

(In reply to Hannes Verschore [:h4writer], pto 13th - 17th of January from comment #3)
> Nice. Does this happen in a known case?

Every benchmark in Octane hits it, yeah.

Comment 5

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d61285b1da5
https://hg.mozilla.org/integration/mozilla-inbound/rev/30d5d70de548

Comment 6

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/abf4a69f46e2: linux32 debug only, https://tbpl.mozilla.org/php/getParsedLog.php?id=32849524&tree=Mozilla-Inbound

Comment 7

[Sean Stangl [:sstangl]]

I don't see how the attached patches could produce segfaults in debugging tests on linux32, and I haven't been able to reproduce any errors locally.

I'll shunt the job to Tryserver again.

Comment 8

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b22524d1e5

Pushed just Part 1. I can't reproduce the failures in Part 2, and it's not worth the time investment.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f3b22524d1e5