1445592 - Generate LOpcodes.h and MOpcodes.h at compile-time

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

The attached patch does this and it seems to work for all platforms (including 'none').

 13 files changed, 89 insertions(+), 1030 deletions(-)

r? nbp for JIT changes, r? nfroyd for build system changes.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Generated MOpcodes.h

This is what the generated MOpcodes.h looks like.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch v2

It just occurred to me we don't need the jit namespace in the generated file because it's a macro, of course.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Generated MOpcodes.h v2

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8958772 [details] [diff] [review]
Patch v2

Review of attachment 8958772 [details] [diff] [review]:
-----------------------------------------------------------------

This is pretty cool!  I have general comments which you are free to ignore, since you are the expert here and I am not!

::: js/src/jit/GenerateOpcodeFiles.py
@@ +26,5 @@
> +  for inputfile in inputs:
> +    for line in open(inputfile):
> +      match = pat.match(line)
> +      if match:
> +        ops.append(match.group('name'))

Do you think it's worth checking for duplicate names here, just to provide early warning to people that something is going to go wrong later?

@@ +39,5 @@
> +    'listname': listname,
> +  })
> +
> +def generate_mir_header(c_out, *inputs):
> +  pat = re.compile(r"^\s*INSTRUCTION_HEADER(_WITHOUT_TYPEPOLICY)?\((?P<name>\w+)\);?$")

Do you think it's worth writing a comment associated with this macro that codegen looks for this macro specifically?

@@ +43,5 @@
> +  pat = re.compile(r"^\s*INSTRUCTION_HEADER(_WITHOUT_TYPEPOLICY)?\((?P<name>\w+)\);?$")
> +  generate_header(c_out, inputs, pat, 'jit_MOpcodes_h', 'MIR_OPCODE_LIST')
> +
> +def generate_lir_header(c_out, *inputs):
> +  pat = re.compile(r"^\s*LIR_HEADER\((?P<name>\w+)\);?$")

Likewise for this one.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8958772 [details] [diff] [review]
Patch v2

Review of attachment 8958772 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/GenerateOpcodeFiles.py
@@ +14,5 @@
> +
> +#ifndef %(includeguard)s
> +#define %(includeguard)s
> +
> +#define %(listname)s(_) \\

nit: Add a comment to mention that the content of this file is generated by jit/GenerateOpcodeFiles.py , this would be helpful for anybody who look at searchfox / dxr.

::: js/src/moz.build
@@ +491,5 @@
>              'jit/x64/SharedIC-x64.cpp',
>              'jit/x64/Trampoline-x64.cpp',
>          ]
>      else:
> +        LOpcodesGenerated.inputs += ['jit/x86/LIR-x86.h']

Nice!

Comment 6

[Jan de Mooij [:jandem]]

Checking for duplicates might be nice to catch copy-paste issues... I'd like to preserve the original order, but I can add a set() in addition to the array, or use an OrderedDict. Even an O(n^2) check on the array is probably fine because we don't have thousands of instructions, but that still feels wrong :)

Good suggestions; I'll post a follow-up patch later today.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Follow-up patch

Here's a patch addressing the review comments. I verified this works as expected.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8958854 [details] [diff] [review]
Follow-up patch

Review of attachment 8958854 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thank you!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd889e36b937
Generate Ion LOpcodes.h and MOpcodes.h instead of updating them manually. r=nbp,froydnj

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/bd889e36b937