Change the OFFSETCLOSURE(2|M2) bytecode instructions to use an offset of 3 instead

[Ekdohibs]

This change is to match the new representation of closures, which has a step of 3 instead of 2.

[gasche]

The change looks obviously correct. There was a special opcode for a common case of field-access-in-a-closure that was detected and generated on the fly. The new closure representation means that the offset-of-interest is 3, not 2 anymore, but the code was not changed when this happened. There was no bug, but the optimization would (probably) not apply anymore so the special opcode was useless. The PR changes the special opcode to apply to offsets of 3 instead of 2, to re-enable the specialized encoding in the same situations.

Questions (not necessarily for @Ekdohibs):

• Can we in fact still see a performance advantage to this specialized instruction? (Presumably the current 'trunk' (and 4.12 ?) state would have a performance regression for bytecode programs that we have not noticed.)
• Does this change in opcode name+semantics affect js_of_ocaml? Should something change there?
• Could we have caught the un-optimization earlier with testsuite support? Should we invest in checking that those custom opcodes are indeed used in the source programs where we believe they should?

[xavierleroy]

Well spotted, and thanks for the patch. It looks good to me.

[Ekdohibs]

Concerning spotting the un-optimization, there are two testcases that seem to be designed those opcodes (t251-pushoffsetclosure2.ml and t251-pushoffsetclosurem2.ml), but where the generated bytecode is only in a comment and not tested for containing the optimized instructions.

[xavierleroy]

Presumably the current 'trunk' (and 4.12 ?) state would have a performance regression for bytecode programs that we have not noticed.

Presumably, although it may be hard to measure. But I guess the code size of the bootstrap compiler (boot/ocamlc) increased when bootstrapping the change of closure representation, as the +2/-2 instructions were no longer generated because the offsets jumped to +3/-3.

Does this change in opcode name+semantics affect js_of_ocaml? Should something change there?

Good question. Who is our liaison agent with js_of_ocaml, again?

Could we have caught the un-optimization earlier with testsuite support?

That's the general issue of CI to monitor performance. There are more egregious cases of performance regression.

Should we invest in checking that those custom opcodes are indeed used in the source programs where we believe they should?

No, this sounds too specific to me. We test for functional correctness first and foremost, and for overall performance if only we knew how to test for that. Testing whether a specific optimization triggers comes as a distant third or fourth.

[nojb]

Does this change in opcode name+semantics affect js_of_ocaml? Should something change there?

Good question. Who is our liaison agent with js_of_ocaml, again?

cc @hhugo

[xavierleroy]

there are two testcases that seem to be designed those opcodes (t251-pushoffsetclosure2.ml and t251-pushoffsetclosurem2.ml)

You're right. I think these tests are supposed to exercise all opcodes, but they are "only" checked for functional correctness (does the generated bytecode work?).

[gasche]

Would it be hard/costly/annoying to just add dynamic checks that the testsuite tests are really generating the opcodes we expect them to? This would catch similar potential issues with the opcodes that are only generated by peephole-style detection.

[xavierleroy]

@Octachron : are you OK with having this change in 4.12? It's not really a bug fix but it corrects an omission in the implementation of the new closure format.

[Octachron]

It sounds indeed better in term of maintenance complexity to not have a gap between the change between the closure format and the associated bytecode instructions.

[xavierleroy]

OK, I'll put it in the 4.12 section of Changes and cherry-pick.