517127 - specialize the add operator

Status: VERIFIED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), enhancement, P3)

Description

[Edwin Smith]

currently: 
   A:* + B:String => concatStrings(string(core,A), B)
proposed:
    => concat_any_str(A, B)

(more cases to be added here)

Comment 1

[Edwin Smith]

Attachment: Refactor convert_s and add to avoid Verifier::emitToString

Verifier::emitToString() is a holdover from before the CodeWriter refactoring, and is only used by the jit.

This patch eliminates it; the type-sniffing optimizations it performed are pushed down into the jit, and consolidated.

the patch removes the extra cases for OP_esc_xattr, OP_esc_xelem, and OP_convert_s from writeOp1.

OP_concat still exists in the code but is dead (it's not a real opcode, it was just used in this bizarre codegen code).  Will remove in a subsequent patch.

Comment 2

[Edwin Smith]

Attachment: removes vestiges of OP_concat

This completes the removal of internal opcode OP_concat.  Note that op_concat has never been a real instruction in abc.

Comment 3

[Edwin Smith]

Attachment: Refactors numeric-add optimization into codegen block for OP_add, remove OP_add_d internal opcode.

This just moves code around and shouldn't affect what we generate.  OP_add_d was never an opcode in ABC, this removes it from code and puts the numeric-add optimization logic further down in the jit.  Where we can also specialize it further without dealing with the CodeWriter abstraction boundary.

Comment 4

[Tom Harwood]

Comment on attachment 401208 [details] [diff] [review]
Refactor convert_s and add to avoid Verifier::emitToString

I'd find the straight STRING_TYPE, NUMBER_TYPE, and OBJECT_TYPE easier to read than st, nt, ot, but it's no biggie.

Comment 5

[Edwin Smith]

no prob, i'll put them back to STRING_TYPE, etc.

Comment 6

[Edwin Smith]

Comment on attachment 401216 [details] [diff] [review]
removes vestiges of OP_concat

pushed http://hg.mozilla.org/tamarin-redux/rev/f7c22610e5ed

Comment 7

[Edwin Smith]

Comment on attachment 401220 [details] [diff] [review]
Refactors numeric-add optimization into codegen block for OP_add, remove OP_add_d internal opcode.

pushed http://hg.mozilla.org/tamarin-redux/rev/71bbb71fd271

Comment 8

[Edwin Smith]

Attachment: adds int specialization to generic op_add code, and moves op_add to instr.cpp

Apart from refactoring, the salient changes are:

* adds int-specialized case at the top of op_add
* op_add takes AvmCore* instead of toplevel; if XML objects are being
  added, then the new xml object is constructed using the toplevel of
  the XML objects passed in, instead of the toplevel of the caller.

  (it seems totally arbitrary, and this saves me passing in an environment).

* boilerplate removed from jit code; we pass in AvmCore* instead of Toplevel*

Comment 9

[Edwin Smith]

this patch is essentially done, and R+, but blocked by the 64-bit-int-atom-precision bug (bug 521353).  landing this bug would make the other one more exposed, by letting it happen in jit code in addition to interpreter code.

opinions?  should this bug be blocked by the other one or should it land anyway?

Comment 10

[Steven Johnson]

(In reply to comment #9)
> opinions?  should this bug be blocked by the other one or should it land
> anyway?

my vote: don't block it, go ahead and land it, and add notes to 521353 to indicate scope creep

Comment 11

[Edwin Smith]

pushed two weeks ago
http://hg.mozilla.org/tamarin-redux/rev/31f739a0ac23

Comment 12

[Chris Peyer]

Engineering work item.  Marking as verified.