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

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
9 years ago
7 years ago

People

(Reporter: gal, Assigned: njn)

Tracking

({perf})

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

9 years ago
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).

Updated

9 years ago
Assignee: general → jseward
(Assignee)

Comment 1

9 years ago
Less LIR means the remaining filters will run faster too.

Which filter is the relevant one -- ExprFilter?  FuncFilter?
(Reporter)

Comment 2

9 years ago
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.
(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 ?
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
Oh, ok.  I thought he meant some cancellation of helper-function
calls, but you're talking about LIR level ops only.
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. :)
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?
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. :-)
(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.
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.
(Reporter)

Comment 11

9 years ago
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.
Ok, that makes much more sense.
(Assignee)

Comment 13

9 years ago
(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?
(Assignee)

Comment 14

8 years ago
(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.
(Assignee)

Comment 15

8 years ago
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.
(Assignee)

Comment 16

8 years ago
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.
(Assignee)

Updated

8 years ago
Depends on: 591539
(Assignee)

Updated

8 years ago
Assignee: jseward → nnethercote
(Assignee)

Updated

8 years ago
Attachment #457719 - Attachment is patch: true
Attachment #457719 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

8 years ago
Attachment #457720 - Attachment is patch: true
Attachment #457720 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 20

7 years ago
TM's days are numbered:  WONTFIX.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.