458277 - Add fast direct-memory-access opcodes to Tamarin-Central

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

Attachment: MOPS patch

Adds a new set of ABC bytecodes to allow for fast, safe direct memory access (but only within specific, per-Domain memory pools allocated specifically for that purpose). 

The new bytecodes don't yet have well-defined public documentation, nor are they generated by the existing free asc.jar compiler; this bug will be updated when documentation becomes available or compiler support is provided.

By default, the new opcodes are enabled; this is controlled by the AVMPLUS_MOPS flag in avmbuild.h. You can disable it by defining AVMPLUS_DISABLE_MOPS on the compiler command line. (MOPS == Memory Ops)

Limitations of the this checkin:
-- The new bytecodes have high-performance code generation in MIR for x86-32 processors, but revert to interpreter on all other builds. (A bug will be entered to add JIT support for them to LIR/nanojit.)
-- There isn’t yet an acceptance test for the new opcodes, as the existing asc.jar compiler doesn’t yet have a way to easily use the new bytecodes. We plan on adding such a test to the standard acceptance suite soon; a new bug will be entered to track this.

Comment 1

[Steven Johnson]

LIR support: https://bugzilla.mozilla.org/show_bug.cgi?id=458279

Acceptance test required: https://bugzilla.mozilla.org/show_bug.cgi?id=458281

Comment 2

[Steven Johnson]

Attachment: MOPS patch #2

Patch is identical to the first except that the stack pop/push values for the new opcodes were incorrect in opcodes.tbl (and thus in opcodes.cpp and opcodes.h); fixed and regenerated these.

Comment 3

[Edwin Smith]

are we planning on the tiered infrastructure for supporting different ABC instruction sets?  if not then i'm not sure we want the AVMPLUS_MOPS #define.

Comment 4

[Edwin Smith]

Comment on attachment 341565 [details] [diff] [review]
MOPS patch #2

This shouldn't land right in TC, we should (at least) integrate & test on LIR.  the rest of these review comments are areas where imho this patch is just not finished to a level required for tc.  no bugs are apparent, however.

are AVMPLUS_MOPS and HAVE_MIR_SMOPS needed?  the problem with new ifdefs is there is a new burden to test both ways unless they're just temporary. and if just temporary, should they be out before landing in tc?

(I can help with the LIR stuff as long as we have good test cases)

the interpreter impl of the new opcodes should have the fast path inlined.

in verifier, use JIT_ONLY() instead of if defined AVMPLUS_MIR || defined FEATURE_NANOJIT

Comment 5

[Steven Johnson]

I'm confused -- LIR is already in TC; I've built & tested this patch against both LIR and MIR (although LIR support amounts to "fail back to the interpreter"). 

AVMPLUS_MOPS indicates whether the interpreter supports the new opcodes or not. I think the idea is that these are permanent; AVMPLUS_MOPS was added as a (temporary) way to allow for testing with or without the support enabled, and it should be removed soon. (Do we want it to be removed before landing? For things of this sort I've usually landed with the ifdef in place, just in case downstream integrators need it temporarily, then removed it in subsequent patches... but I could be presuaded otherwise)

HAVE_MIR_SMOPS indicates whether MIR has explicit support for jitting these new opcodes -- currently this is only the case for x86-32.

re: testcases, we have a set of testcases that are essentially a handcoded ABC file -- I'll attach them to this bug for anyone who wants to try them.

re: fast-path inlined, I'm not sure I understand what you mean -- pull AvmCore::integer and AvmCore::intToAtom into the ops?

re: JIT_ONLY, sure, I can do that. BTW, shouldn't we have an AVMPLUS_JIT (== defined AVMPLUS_MIR || defined
FEATURE_NANOJIT) ?

Comment 6

[Edwin Smith]

(In reply to comment #5)
> I'm confused -- LIR is already in TC

what i mean is to have LIR actually on par with MIR.  (not a small task but not huge either)

> HAVE_MIR_SMOPS indicates whether MIR has explicit support for jitting these new opcodes -- currently this is only the case for x86-32.

oh, i didn't realize MIR didn't support the new ops uniformly.  more todo's then.

> re: testcases, we have a set of testcases that are essentially a handcoded ABC
> file -- I'll attach them to this bug for anyone who wants to try them.

we can still commit the binary test cases for lack of anything nicer.

> re: fast-path inlined, I'm not sure I understand what you mean -- pull
> AvmCore::integer and AvmCore::intToAtom into the ops?

if that's the fast path, sure.  check out (say) OP_add for an example.  if the interpreter can avoid calling out to env for the common cases, its a big win.  perhaps this is a followon task (cue tape on not landing unfinished work into tc)

> re: JIT_ONLY, sure, I can do that. BTW, shouldn't we have an AVMPLUS_JIT (==
> defined AVMPLUS_MIR || defined
> FEATURE_NANOJIT) ?

now that you mention it, good idea, altho not a prerequisite for this patch.  (having FEATURE_NANOJIT exposed to embedders was a mistake).

Comment 7

[Steven Johnson]

re: MIR support, frankly I'm not sure it's worth it, since we're moving aggressively towards LIR anyway. I think we should focus on implementing them in LIR and leave the MIR support as-is for legacy purposes.

re: TC vs redux, I'll work on a revised patch that address some of the above (eg interpreter fastpath) and offer it vs redux instead of TC.

re: AVMPLUS_JIT, I won't attempt to deal with that in this patch at all (I'm guessing you or Lars will take that up in redux).

Comment 8

[Steven Johnson]

(In reply to comment #4)

> in verifier, use JIT_ONLY() instead of if defined AVMPLUS_MIR || defined
> FEATURE_NANOJIT

There lot's of places in Verifier where we do this instead of JIT_ONLY -- basically, anywhere we have a multiline conditional.... I'm just following existing precedent here.

Comment 9

[Steven Johnson]

Attachment: MOPS Patch #2

Adds a new set of ABC bytecodes to allow for fast, safe direct memory access
(but only within specific, per-Domain memory pools allocated specifically for
that purpose). 

The new bytecodes don't yet have well-defined public documentation, nor are
they generated by the existing free asc.jar compiler.

By default, the new opcodes are enabled; this is controlled by the AVMPLUS_MOPS
flag in avmbuild.h. You can disable it by defining AVMPLUS_DISABLE_MOPS on the
compiler command line. (MOPS == Memory Ops)

Limitations of the this checkin:

-- The new bytecodes have high-performance code generation in MIR for x86-32
processors; all other architectures (and all LIR builds) simply generate function calls for the new opcodes when jitting. https://bugzilla.mozilla.org/show_bug.cgi?id=458279
 has been entered to track the task of adding better support to LIR. (At this point it's probably not worth the effort to upgrade MIR for other architectures.)

-- There is an acceptance test for the new opcodes, but it is in binary-only form (test/acceptance/mops/mops.abc) as a suitable compiler isn't yet publicly available. (Note, this test also outputs its runtime at exit so it can be used as a rough performance test as well.) https://bugzilla.mozilla.org/show_bug.cgi?id=458281 has been entered to track the lack of proper source code.

This patch has been pushed to the user branch http://hg.mozilla.org/users/stejohns_adobe.com/tamarin-mops/ as changeset a3aa205c976f; though this code is reliable and stable, we'd like to get proper LIR support for the new opcodes before migrating it into TC, so further work will be done in the user branch above for now. (We'd also like to have a better set of acceptance/performance tests but might be possible to bend on that requirement.)

Comment 10

[Steven Johnson]

pushed to redux as changeset:   1004:e9e705244c92

Comment 11

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.