915228 - don't generate a static constructor for ARM's DoubleEncoder

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

This doesn't completely eliminate a static constructor from Assembler-arm.cpp,
but it at least moves some computation from run-time to compile-time, which
IMHO is always a good thing.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - make DoubleEntry and field types MOZ_CONSTEXPR

MOZ_CONSTEXPR is necessary so our compilers on ARM (i.e. GCC) will properly
fold away constructors into constant data.  The initialization of pad is
because constexpr requires that all fields of a structure be initialized in
a constexpr constructor.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - make DoubleEncoder's table statically defined

This is the real meat: translate C algorithm into Python, generate the table,
include the table, and blow away all the C code to compute the table.  I didn't
think it necessary to hook all this up to the build process, given that we're
unlikely to modify gen-double-encoder-table.py.

I think I got the references to the ARM ARM right; I'm guessing that we're using
the F32 encoding extended to doubles.  I'd appreciate clarification on that.

Comment 3

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 803089 [details] [diff] [review]
part 2 - make DoubleEncoder's table statically defined

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

::: js/src/jit/arm/Assembler-arm.cpp
@@ +1218,5 @@
>  }
>  
> +const js::jit::DoubleEncoder::DoubleEntry js::jit::DoubleEncoder::table[256] = {
> +#include "DoubleEntryTable.inc"
> +};

As much as I dislike the idea of having a #include in the middle of an expression, this is probably best, since 99% of all code inspection tools will correctly find this definition.

::: js/src/jit/arm/gen-double-encoder-table.py
@@ +15,5 @@
> +def rep(bit, count):
> +    return reduce(operator.ior, [bit << c for c in range(count)])
> +
> +def encode(value):
> +    """Generate an ARM ARM 'VFP modified immediate constant' with format:

You should name this 'encodeDouble' or something like that, since the new float32 work may necessitate generating a similar table for single precision floats..

@@ +18,5 @@
> +def encode(value):
> +    """Generate an ARM ARM 'VFP modified immediate constant' with format:
> +    aBbbbbbb bbcdefgh 000...
> +
> +    This is from Table A7-15 in the ARM ARM 406B, F32 format, extended to

The table is actually A7-18 (at least in my copy of the ARM ARM). Since the section numbers, and table layouts can change from one revision to the next, putting direct references in is generally discouraged.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #3)
> @@ +18,5 @@
> > +def encode(value):
> > +    """Generate an ARM ARM 'VFP modified immediate constant' with format:
> > +    aBbbbbbb bbcdefgh 000...
> > +
> > +    This is from Table A7-15 in the ARM ARM 406B, F32 format, extended to
> 
> The table is actually A7-18 (at least in my copy of the ARM ARM). Since the
> section numbers, and table layouts can change from one revision to the next,
> putting direct references in is generally discouraged.

I was trying to sidestep that by listing the exact revision. =/  I'll take it out.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #3)
> @@ +18,5 @@
> > +def encode(value):
> > +    """Generate an ARM ARM 'VFP modified immediate constant' with format:
> > +    aBbbbbbb bbcdefgh 000...
> > +
> > +    This is from Table A7-15 in the ARM ARM 406B, F32 format, extended to
> 
> The table is actually A7-18 (at least in my copy of the ARM ARM). Since the
> section numbers, and table layouts can change from one revision to the next,
> putting direct references in is generally discouraged.

On IRC, we determined that I was actually looking at the wrong table: it ought to be A7-18, and it is the same table in both of our revisions.  Perhaps the table numbers are stable enough to be used as references?  However, Marty said that you (Jacob) had encouraged him to not to put in references for...other things.  So, Jacob, would you mind shedding some light on whether the table numbering is stable across revisions and/or why you would encourage references to not be used for particular things?

Comment 6

[Jacob Bramley [:jbramley]]

(In reply to Nathan Froyd (:froydnj) from comment #5)
> So, Jacob, would you mind shedding some
> light on whether the table numbering is stable across revisions and/or why
> you would encourage references to not be used for particular things?

The table numbers, chapter numbers and so on might change between revisions, that is true. They're numbered sequentially so if any sections (or tables) are added or removed, the numbering of subsequent sections (or table) changes. You can reference a specific revision using the document number. In your case, "ARM DDI 0406B" is what is shown on the document, but "ARM ARM 406B" is good enough for these purposes. The numbering should not change for a given document revision.

Having said that, I do generally discourage recording references into the ARM ARM for a couple of reasons:
  - It's not helpful to annotate instruction encodings with ARM ARM links because almost everything of interest to a JIT back-end can be found by navigating through the instruction set listing. For example, if you look up the VMOV <imm> instruction, there's a link to the table that describes the immediate encoding anyway.
  - Whilst the document number uniquely identifies the document, the reader of the code most likely has a different version. For example, the latest released ARMv7-AR ARM is issue C (0406C). If that is the case, the reference isn't very useful anyway.
    - It's useful in a record of history (such as a commit message), but not in something that needs to be useful in the future and kept up to date (like a code comment).

Comment 7

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a09266fa0ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c789a0ddda51

Pushed with review comments addressed and small typos in the generation script addressed so it won't crash (unfixed try run: https://tbpl.mozilla.org/?tree=Try&rev=4b2d345d0800).  Green try run:

https://tbpl.mozilla.org/?tree=Try&rev=74ec4ed39d72

Comment 8

[Nathan Froyd [:froydnj]]

And backed out, because check-spidermonkey-style.py doesn't like .inc files.

OK to just change the file to a .h file?

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 2 - make DoubleEncoder's table statically defined

Actually, here, just review a new patch with .tbl instead.

Comment 10

[Nathan Froyd [:froydnj]]

Hm, this patchset causes timeouts during B2G emulator tests:

https://tbpl.mozilla.org/?tree=Try&rev=b5da39519b91

:(

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #10)
> Hm, this patchset causes timeouts during B2G emulator tests:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=b5da39519b91
> 
> :(

Vicamo, ahal said you've debugged some qemu issues before.  Do the above hangs/crashes look similar to the issues you've encountered?  I suspect qemu here, because real hardware works just fine with the changes (other try pushes not shown here).

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Nathan Froyd (:froydnj) from comment #11)
> Vicamo, ahal said you've debugged some qemu issues before.  Do the above
> hangs/crashes look similar to the issues you've encountered?  I suspect qemu
> here, because real hardware works just fine with the changes (other try
> pushes not shown here).

Ting Yuan's expertise on ARM assembly would help more.  I have barely enough knowledge to judge here, but it could simply be an intermittent failure of B2G emulator which we have had a lot since it has been on the stage.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #12)
> (In reply to Nathan Froyd (:froydnj) from comment #11)
> > Vicamo, ahal said you've debugged some qemu issues before.  Do the above
> > hangs/crashes look similar to the issues you've encountered?  I suspect qemu
> > here, because real hardware works just fine with the changes (other try
> > pushes not shown here).
> 
> Ting Yuan's expertise on ARM assembly would help more.  I have barely enough
> knowledge to judge here, but it could simply be an intermittent failure of
> B2G emulator which we have had a lot since it has been on the stage.

The odd thing is that the assembly generated here doesn't change; the patch is changing a table of constants, and the constants are the same before and after.  So I'm not sure what's causing the emulator to trip up.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: eliminate static constructors for DoubleEncoder's table

I took another run at this with the recent decommissioning of Android 2.2
builds, since I thought those were what was holding this patch back from
landing.  I was forcefully reminded that all the problems were on the B2G end.

This patch is the result.  I don't particularly like the hack required for
Imm8VFPImmData; the comment present there explains the rationale.  Once we have
B2G builds on a reasonable compiler, I commit to going back and doing it the
right way with the previous patches in this bug.  Given that Imm8VFPImmData is
only used in a few places, I think it's OK to take away the safety of the
constructors.

Comment 15

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8cd71a4dc0c

Comment 16

[Nicholas Nethercote [inactive]]

> https://hg.mozilla.org/integration/mozilla-inbound/rev/e8cd71a4dc0c

That commit is missing some files: DoubleEntryTable.tbl and gen-double-encoder-table.py

Comment 17

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6dcbdd9e9846

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/e8cd71a4dc0c
> 
> That commit is missing some files: DoubleEntryTable.tbl and
> gen-double-encoder-table.py

Oof, that's what I get for trying to land patches late at night.  Relanded with appropriate |hg add|:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd4f1368b7a

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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