525437 - clean up LIR_call

Status: RESOLVED FIXED

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

Description

[Edwin Smith]

Proposal to clean up encoding of LIR_call (icall, fcall, etc).

1. Move argument references into a separate chunk of memory allocated by LirBuffer._allocator (thus guaranteeing same lifetime as LIR).  This makes the LInsC fixed sized.

2. Drop LInsC.argc since it is redundant with CallInfo and not needed for fast traversal of LirBuffer anymore.  For such a small field (4 bits max) its a waste to use up a whole redundant field.

There is also redundancy between the opcode and CallInfo._argtypes, but I think that's worth preserving so that the opcode by itself indicates the type of the call result.  (Although one could argue that type checking arguments requires decoding argtypes anwyay).

Comment 1

[Andreas Gal :gal]

Yeah, sounds good. We do that for our type maps too.

Comment 2

[Nicholas Nethercote [inactive]]

Both (1) and (2) sound good.

In particular, making all LIR instructions a fixed size should help speed up LirReader::read(), which has a non-negligible compile-time cost.  (See bug 513865 for a prior attempt at speeding it up which didn't end up helping... with this change it has a better chance of working.)  It may also simplify LirBuffer::makeRoom().

For the return type, another option is to encode it in the opcode but not in the _argtypes.  That would make _argtypes a more accurate name.  It would also free up 3 more bits for additional CallInfo properties.

Also, do we need an opcode for a function with void return type?

I'm happy to make the changes (I've assigned it to myself).  But we need a Nanojit/Nanojit product/component in Bugzilla.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch does (1) and (2), but none of the other suggested changes.

Comment 4

[Nicholas Nethercote [inactive]]

I'm seeing a 9ms SS slowdown but I think it might just be noise, I'll measure again when a few more commits have landed.

Comment 5

[Andreas Gal :gal]

I wonder whether we could fix the order of arguments for insCall. I know its a ton of code movement but ... well it would really just plain make sense.

Comment 6

[Nicholas Nethercote [inactive]]

Yeah, I'd love to, it's ridiculous as is... but perhaps in a separate bug.  Is there any reason for doing it backwards?

Comment 7

[Andreas Gal :gal]

Yeah, def separate bug. I would bet the reverse reading made it more convenient at some point.

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
> Yeah, def separate bug. I would bet the reverse reading made it more convenient
> at some point.

Added as bug 525815.

Comment 9

[Edwin Smith]

Comment on attachment 409652 [details] [diff] [review]
patch

I also like the idea of keeping the different opcodes and eliminating returntype from the argtypes array.

one gotcha though is on 64bit machines, dealing with functions that return int32 or uint32.

if the caller needs to know whether to do sign extension or zero extension then that information needs to be recorded somewhere.

if on the other hand the abi always ensures the result will be extended to int64_t or uint64_t by the callee, then we dont need the information.

(the only two 64bit architectures we support are x64 and ppc64 so this just needs a quick checkup before we do the coding.

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> (From update of attachment 409652 [details] [diff] [review])
> I also like the idea of keeping the different opcodes and eliminating
> returntype from the argtypes array.

Filed as bug 526365.  I filed it as a follow-on because (a) I have r+ for the existing patch, and (b) it requires modifying Tracemonkey and Tamarin, whereas the current patch for this bug is a nanojit-only change.

Comment 11

[Nicholas Nethercote [inactive]]

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

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/807daff04870