572798 - add LIR_callv

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Comment 2

[Edwin Smith]

I found a bug in TR's frontend, that the ValidateWriter could have caught, if it knew about ARGTYPE_V return types.  Changing LIns->retType() like this catches the bug:

         LTy retType() const {
-            return retTypes[opcode()];
+            if (isCall() && callInfo()->returnType() == ARGTYPE_V)
+                return LTy_V;
+            else
+                return retTypes[opcode()];
         }
         bool isV() const {
             return retType() == LTy_V;

Now, on the one hand, we don't want this, we want the new opcode.   On the other hand, retType() is *only* called by the type checker, and a new opcode is a much bigger change.  Opinions, anyone?

Comment 3

[Edwin Smith]

Attachment: LIns.retType() returns LTy_V if CallInfo->returnType() == ARGTYPE_V

Here's the simple fix that improves ValidateWriter (only) without a new opcode.  If we'd rather have the opcode, R- and I'll work on a bigger patch.  I'm posting this minimal patch since it was enough to catch one subtle TR bug.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 476265 [details] [diff] [review]
LIns.retType() returns LTy_V if CallInfo->returnType() == ARGTYPE_V

Let's do it properly!  It shouldn't be that much harder -- there are only 7 occurrences of LIR_addq in Nanojit, so LIR_addv should be similar.

Comment 5

[Edwin Smith]

Okay, just the nudge I needed.  I'll work on it.  I think you meant call, not add.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
> I think you meant call, not add.

Indeed, I meant LIR_callq.

Comment 7

[Edwin Smith]

Attachment: Add LIR_callv for void calls

Adds LIR_callv for calls to other functions that return void.

Added a ValidateWriter check that LIR_callv to be paired with ARGTYPE_V.

getCallOpcode() returns LIR_callv for ARGTYPE_V, as expected.  This means that some calls will return LTy_V from LIns::retType(), as expected, but unlike before.  This in turn can cause a ValidateWriter error if an instruction uses the result of a void call.  (after all, that's the point).

Each backend was modified to not assign a register or save the result of a void call.

Comment 8

[Edwin Smith]

Attachment: (TR) Handle LIR_callv in deadvars_analyze/kill().

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 477605 [details] [diff] [review]
Add LIR_callv for void calls

>+        if ((op == LIR_callv) != (ci->returnType() == ARGTYPE_V)) {
>+            NanoAssertMsgf(0,
>+                "LIR structure error (%s): return type mismatch: opcode %s with %s return type",
>+                whereInPipeline, lirNames[op],
>+                ci->returnType() == ARGTYPE_V ? "void" : "non-void");
>+        }
>+

We should do this kind of checking for all call kinds, ie. that the retType
in the CallInfo is compatible with the opcode.  I realize it's scope creep
for this patch, but it's minor -- would you mind adding it?


> //---------------------------------------------------------------------------
> // Calls
> //---------------------------------------------------------------------------
> OP___(calli,    33, C,    I,   -1)  // call subroutine that returns an int
> OP_64(callq,    34, C,    Q,   -1)  // call subroutine that returns a quad
> OP___(calld,    35, C,    D,   -1)  // call subroutine that returns a double
>-
>-OP_UN(36)
>+OP___(callv,    36, C,    V,   -1)  // call subroutine that returns void

Nit: I'd probably put callv before calli, but I'll leave it up to you.

Nit: whatever order you choose, can you make the various switches that involve
callv have their cases in the same order as here?


The rest looks good, r=me with the above points addressed, no need to
re-review.  Thanks!

Comment 10

[Edwin Smith]

I'll add the extra checks, and fix the case ordering.  You never know what you'll find until you look.  Better to look now than the day after pwn20wn.

I also wanted to add one more check, now that we broke the seal on scope creep:  a LIR_callv shouldn't call a pure function.  (think about it...)

Comment 11

[William Maddox]

Comment on attachment 477606 [details] [diff] [review]
(TR) Handle LIR_callv in deadvars_analyze/kill().

R+

Comment 12

[William Maddox]

http://hg.mozilla.org/tamarin-redux/rev/1274a4de6df1

http://hg.mozilla.org/tamarin-redux/rev/aaad459b5cee

Comment 13

[William Maddox]

http://hg.mozilla.org/projects/nanojit-central/rev/f1076b3fa633

Comment 14

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/df05d03542f7

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/df05d03542f7

Comment 16

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Ts, Cold MED Dirty Profile regression on Firefox. boo-urns :(

  Previous: avg 420.356 stddev 3.679 of 30 runs up to a9d1ad0bc386
  New     : avg 430.832 stddev 1.905 of 5 runs since a60414d076b5
  Change  : +10.475 (2.49% / z=2.847)
  Graph   : http://mzl.la/bZFaB3

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-ts_cold_generated_med] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 17

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Win7 Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2014.419 stddev 40.480 of 30 runs up to a9d1ad0bc386
  New     : avg 1901.610 stddev 12.432 of 5 runs since a60414d076b5
  Change  : -112.809 (-5.6% / z=2.787)
  Graph   : http://mzl.la/9gFu4n

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 18

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Win7 Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 127.610 stddev 4.222 of 30 runs up to a9d1ad0bc386
  New     : avg 116.384 stddev 0.751 of 5 runs since a60414d076b5
  Change  : -11.226 (-8.8% / z=2.659)
  Graph   : http://mzl.la/bZu5UP

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 19

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2045.275 stddev 49.676 of 30 runs up to a9d1ad0bc386
  New     : avg 1936.120 stddev 13.937 of 5 runs since a60414d076b5
  Change  : -109.155 (-5.34% / z=2.197)
  Graph   : http://mzl.la/b0dlou

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 20

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 129.703 stddev 4.099 of 30 runs up to a9d1ad0bc386
  New     : avg 117.954 stddev 0.660 of 5 runs since a60414d076b5
  Change  : -11.749 (-9.06% / z=2.866)
  Graph   : http://mzl.la/avZij4

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.