505876 - TM: eliminate redundant box/unbox operations when emitting code, not in a filter

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

We currently remove redundant box/unbox operations, by always emitting i2f at the end of every integer op, and then matching it up with the pairing f2i in the beginning of another arithmetic op in a filter. f2i(i2f) is then eliminated. This should be done directly when emitting the code, i.e. by looking back into the lir and checking whether the result is quad or not. Since this only happens for numbers, for integers it will always be !quad, and for doubles it will always be quad. This way we can get rid of the filter, and we no longer have to recognize f2i and i2f in the LIR, which will allow us to inline them as LIR instead of having to use calls for both of them (only so we can match ->callInfo() == f2i).

Comment 1

[Nicholas Nethercote [inactive]]

Less LIR means the remaining filters will run faster too.

Which filter is the relevant one -- ExprFilter?  FuncFilter?

Comment 2

[Andreas Gal :gal]

FuncFilter is the one where the logic is hidden. So this is all in the main tracer. We don't emit any of this lir, its all filtered, but its less virtual method dispatching around.

Comment 3

[Julian Seward [:jseward]]

(In reply to comment #0)
Looking at this now.  Am I right to understand what you mean is,
move the matching of such pairs from FuncFilter into all the back
ends?  If yes, won't that interact badly with the downstream
optimisations, particularly CSE (which is now going to have to
CSE up function calls) and constant folding ?

Comment 4

[Mike Shaver (:shaver emeritus)]

I believe that what Andreas means is that when emitting f2i, we should peek at the operand, and if it's an i2f, instead just use the operand of the i2f.  I don't think you even need to patch the stream necessarily, just have f2i return the operand rather than generating a new one?  If I'm right, it's trivial. :)

LIns *numins = /* whatever */
LIns *somenum = i2f(numIns);
LIns *asIndex = f2i(somenum); // asIndex should just be numIns here

Comment 5

[Julian Seward [:jseward]]

Oh, ok.  I thought he meant some cancellation of helper-function
calls, but you're talking about LIR level ops only.

Comment 6

[Mike Shaver (:shaver emeritus)]

If my plan works, I'm sure Andreas will be happy with it.  If it doesn't, then he will of course have meant something different. :)

Comment 7

[Julian Seward [:jseward]]

Hmm, the more I look at this, the less I understand it (this may
merely mean I'm in the "increasing confusion" stage of problem
solving).

Firstly there is no LIR_f2i, only i2f/u2f; float->int conversions
are done by function calls it seems.  (No big deal)

Secondly, FuncFilter::insCall(const CallInfo *ci, LInsp args[])
has this:

        if (ci == &js_DoubleToUint32_ci) {
            LInsp s0 = args[0];
        ...
            if (isi2f(s0) || isu2f(s0))
                return iu2fArg(s0);

This appears to allow

  js_DoubleToUInt32( LIR_u2f(x) ) --> x
  js_DoubleToUInt32( LIR_i2f(x) ) --> x

First seems reasonable, but the second doesn't sound right when x is
negative.  What am I missing?

Comment 8

[Mike Shaver (:shaver emeritus)]

I don't think that you're missing anything, may be a latent bug.  I do wish you would stop finding those.

The first case would seem a good place to start, though, if we all agree that it's reasonable. :-)

Comment 9

[Julian Seward [:jseward]]

(In reply to comment #8)
re  js_DoubleToUInt32( LIR_i2f(x) ) --> x
I imagine, from staring for too long
at http://bclary.com/2004/11/07/#a-9.5 and http://bclary.com/2004/11/07/#a-9.6,
that it might be possible to construct an argument that this is in fact a
correct transformation.  Let's see what Andreas has to say.

> The first case would seem a good place to start, though, if we all agree that
> it's reasonable. :-)

Yep.

Comment 10

[Julian Seward [:jseward]]

Sorry to be a pain, but .. I don't think I understand the point.
(it may be that I still don't understand the proposal).

Currently all this merging logic (basically, expression-tree rewriting)
is in FuncFilter::insCall.  And the proposal is to pull it out of 
FuncFilter::insCall and do it earlier. (?)  But FuncFilter::insCall is 
the first filter in the writer pipeline anyway.  So it's not as if this
would make the transformed LIR available to any more pipeline stages than
it is at the moment.  And it would split the logic in FuncFilter
into two different pieces of code, since FuncFilter deals with more
than just f2i(i2f(x)), so FuncFilter couldn't be removed even if
this particular transformation was moved somewhere else.

Comment 11

[Andreas Gal :gal]

Yes, there is no LIR_f2i, because f2i is lossy and the precise semantics of it is VM-defined. The jit only provides i2f, since thats lossless.

Filters only have access to LIR. They have to make decisions based on looking at LIR. In LIR, you can't tell on a 64-bit platform whether a value is a 64-bit pointer (quad), or a double (quad), because both are ... quads.

The proposal is to no longer always convert numbers to floats. If you do a math op, leave it an integer. The next op will look at its arguments, and if the type is number (i.e. isNumber(sp[-1])), then it will look at the lir whether its a quad or an int (tracker(sp[-1])->isQuad()) and if it wants float (i.e. fadd) it will do i2f. If it wants an integer, it just keeps it as an integer. If its currently a float and it wants an int (i.e. <<) then it will emit DoubleToECMAInt32.

Comment 12

[Julian Seward [:jseward]]

Ok, that makes much more sense.

Comment 13

[Nicholas Nethercote [inactive]]

(In reply to comment #11)
> Yes, there is no LIR_f2i, because f2i is lossy and the precise semantics of it
> is VM-defined. The jit only provides i2f, since thats lossless.

f2i has since been added.  I think it's true that f2i(i2f(x))==x and i2f(f2i(x))==x?

> Filters only have access to LIR. They have to make decisions based on looking
> at LIR. In LIR, you can't tell on a 64-bit platform whether a value is a 64-bit
> pointer (quad), or a double (quad), because both are ... quads.

You can now, I64s and F64s are properly separated.  Does this change things much?

> The proposal is to no longer always convert numbers to floats. If you do a math
> op, leave it an integer. The next op will look at its arguments, and if the
> type is number (i.e. isNumber(sp[-1])), then it will look at the lir whether
> its a quad or an int (tracker(sp[-1])->isQuad()) and if it wants float (i.e.
> fadd) it will do i2f. If it wants an integer, it just keeps it as an integer.
> If its currently a float and it wants an int (i.e. <<) then it will emit
> DoubleToECMAInt32.

Do you expect this to improve generated code much?  Or is it just meant to simplify/optimise compilation?

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to comment #13)
> 
> f2i has since been added.  I think it's true that f2i(i2f(x))==x and
> i2f(f2i(x))==x?

The former is true, the latter is not.

> Do you expect this to improve generated code much?  Or is it just meant to
> simplify/optimise compilation?

AIUI it's just the latter.

Comment 15

[Nicholas Nethercote [inactive]]

I started looking into this.  I quickly got dragged into the slot-handling code -- I'm getting loops considered as type-unstable that clearly are stable -- which I don't understand at all.  This is quite an all-or-nothing change.

Comment 16

[Nicholas Nethercote [inactive]]

Yeah, this is a big change that I don't think has a good cost/benefit ratio
right now.  If recursion in the tracer gets removed after JM is integrated,
then that would simplify SlotMap and it might be worthwhile.

In the meantime, I'm posting two patches.  The first one is a clean-up patch
that doesn't change functionality:
- Renames f2u() as d2u()
- Stops handling intops in alu(), renames it accordingly as alu_double()
- Inverts isInstructionUndemotable() to isInstructionDemotable(), which
  better fits how it's used
- A few other minor tweaks.

The second one starts to do the real changes.  It's totally broken, but is a
starting point.  The third one is another attempt, also broken.  Any future
work should cherry-pick useful pieces from the two of them.

Both patches are against TM rev 47533:c8a0f367bb32.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: patch 1

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: patch 2

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: patch 3

Comment 20

[Nicholas Nethercote [inactive]]

TM's days are numbered:  WONTFIX.