1526425 - Convert CacheIR opcode list into table with argument formats

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Iain Ireland [:iain]]

I noticed while working on CallICs that our infrastructure for walking through CacheIR bytecode is pretty bad. The only way to tell that the CacheIRWriter wrote out as many arguments as the CacheIRCompiler expects is to run the tests and check for hilarious crashes. This is a problem if we want to walk through the bytecode in any other context, because we have no way of knowing how many bytes of arguments we should skip to get to the next bytecode. In essence, our opcodes are dynamically typed.

To handle this, we have a pile of ad-hoc code in BaselineInspector to carefully skip the right number of bytes in the specific cases we know that we care about. That's not a viable long-term plan.

(This state of affairs also makes it hard to, say, print out a dump of a bytecode sequence. Introspection!)

After talking it over with Matt, I've settled on the following approach:

1. Instead of being a simple list of opcodes, CACHE_IR_OPS now includes format specifiers. For example, GuardIsObject has CacheFormat::Id, indicating that the argument is a single OperandId.
2. Format specifiers are either Nullary (no operands) or a list of argument types separated by _. The current argument types are:
   • Id = any operand id
   • Field = stub field index
   • Byte = inline byte
   • Int32 = inline int32_t
   • UInt32 = inline uint32_t
   • Word = inline word-sized pointer
3. To keep the number of formats down, I'm establishing a convention that arguments are written in the above order: Ids first, then Fields, then Bytes, then Int32/Uint32/Word. Many/most of the existing opcodes already followed this convention. I've rewritten the ones that didn't. (The lone exception is AllocateAndStoreDynamicSlot with (deep breath) CacheFormat::Id_Id_Field_Field_Field_Byte_Field, which shares code with AddAndStore(Fixed|Dynamic)Slot but includes an extra Field.)
4. Having a format for each opcode allows us to look up the length of the operands. This lets us assert during compilation that emitFoo consumed the correct number of bytes.
5. This also makes it much easier to pretty-print CacheIR bytecode.
6. As a follow-up, the ad-hoc code in BaselineInspector can be standardized.

Comment 2

[Jan de Mooij [:jandem]]

Great idea. I've wanted to do something like this since the early CacheIR days but never got around to it.

Comment 3

[Iain Ireland [:iain]]

I implemented steps 1-5 above, but was concerned that my approach ended up adding mechanical busywork to any new CacheIR opcode that didn't match an existing format. After a slight rework:

1. Instead of hardcoding all the different possible formats, I use a little bit of template magic to parse the list. Instead of: |_(AllocateAndStoreDynamicSlot, Id_Id_Field_Field_Field_Byte_Field)| we get to write: |_(AllocateAndStoreDynamicSlot, Id,Id,Field,Field,Field,Byte,Field)|. The big advantage is that we no longer have to add a switch case to handle printing / finding the length of Id_Id_Field_Field_Field_Byte_Field. The templates handle it auto-magically for us.

2. Because we don't have a hard-coded list of formats, we no longer need to standardize on a single argument order. I reverted that part of my patch to avoid unnecessary churn.

3. I've extended CACHEIR_LOGS to dump the instruction sequence as JSON as part of trackAttached. For example, here's a SetProp:

"cacheIR":[{ "op":"GuardIsObject", "args":[{ "Id":0 }] },
           { "op":"GuardGroup",    "args":[{ "Id":0 }, { "Field":0 }] },
           { "op":"GuardShape",    "args":[{ "Id":0 }, { "Field":1 }] },
           { "op":"AllocateAndStoreDynamicSlot",
                 "args":[{ "Id":0 }, { "Field":2 }, { "Id":1 }, { "Byte":0 },
		         { "Field":3 }, { "Field":4 }, { "Field":5 }] }]

There's not a lot of information about what the arguments mean, but it's better than nothing, with room for future improvement.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1526425: Convert CacheIR opcode list into table with argument formats r=mgaudet

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1526425: Add debug verification of CacheIR operand length r=mgaudet

Depends on D19832

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1526425: Add bytecode output to CacheIRSpewer r=mgaudet

Depends on D19833

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2d6b8206890c
https://hg.mozilla.org/mozilla-central/rev/af9a66497c52
https://hg.mozilla.org/mozilla-central/rev/093f32c38e3a