Closed Bug 1115748 Opened 5 years ago Closed 5 years ago

x86 assembler spew/encoding consistency

Categories

(Core :: JavaScript Engine: JIT, enhancement)

x86_64
All
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(7 files)

While working on finishing the VEX encodings, I found it useful to compare the results of BaseAssembler-x86-shared.h's encoding with the results of running its spew output through a standalone mature x86 assembler.

One of the things this turned up was numerous syntax errors and other minor issues in the spew output. The following patch series corrects most of the errors, allowing the spew output to be fed to an assembler with minimal preprocessing.

There are a bunch of changes here, but they're uncomplicated, so hopefully they'll be easy to review.
Attachment #8541746 - Flags: review?(jdemooij)
This patches takes the PRETTY_PRINT_OFFSET macro to the next level: macros for consistent formatting of memory addresses.
Attachment #8541747 - Flags: review?(jdemooij)
Fix spewing of 8-bit register names.
Attachment #8541755 - Flags: review?(jdemooij)
This is just a minor code cleanup.
Attachment #8541758 - Flags: review?(jdemooij)
Consistency and cleanup in the handling of immediate values.
Attachment #8541762 - Flags: review?(jdemooij)
This patch just assigns enum values to the VEX prefix bytes.
Attachment #8541770 - Flags: review?(jdemooij)
This updates more syntax to be parsable by GAS and llvm-mc. It replaces ? and ((...)) with .Lfrom labels which can be directly parsed and handled.
Attachment #8541779 - Flags: review?(jdemooij)
Attachment #8541746 - Flags: review?(jdemooij) → review+
Comment on attachment 8541747 [details] [diff] [review]
assembler-spew-mem.patch

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

Nice, this also looks a lot simpler to use when adding new instructions.

::: js/src/jit/shared/BaseAssembler-x86-shared.h
@@ +1184,5 @@
>  
>      void subl_im(int imm, int offset, RegisterID base)
>      {
> +        spew("subl       $%d, " MEM_ob,
> +             imm, ADDR_ob(offset, base));

Nit: probably fits on one line now.

@@ +1259,5 @@
>  
>      void xorl_rm(RegisterID src, int offset, RegisterID base)
>      {
> +        spew("xorl       %s, " MEM_ob,
> +             nameIReg(4,src), ADDR_ob(offset, base));

Same here and below.

@@ +1570,5 @@
>  
>      void cmpl_im(int rhs, int offset, RegisterID base)
>      {
> +        spew("cmpl       $0x%x, " MEM_ob,
> +             rhs, ADDR_ob(offset, base));

And here.
Attachment #8541747 - Flags: review?(jdemooij) → review+
Comment on attachment 8541755 [details] [diff] [review]
assembler-norex-names.patch

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

::: js/src/jit/shared/BaseAssembler-x86-shared.h
@@ +128,5 @@
>  
> +    static const char* nameI8Reg_norex(RegisterID reg)
> +    {
> +        // The same as r8names above, except that %spl through %dil are replaced
> +        // by %ah through %bh since there is to be no REX prefex.

Nit: prefix (typo)

@@ +133,5 @@
> +        static const char* const r8names_norex[16]
> +          = { "%al", "%cl", "%dl", "%bl", "%ah", "%ch", "%dh", "%bh",
> +              "%r8b", "%r9b", "%r10b", "%r11b", "%r12b", "%r13b", "%r14b", "%r15b" };
> +        int off = (RegisterID)reg - (RegisterID)eax;
> +        return (off < 0 || off > 15) ? "%r???" : r8names_norex[off];

This matches the other method, but could we assert `off` is in range here and in nameFPReg / nameIReg? Or will it break something?
Attachment #8541755 - Flags: review?(jdemooij) → review+
Attachment #8541758 - Flags: review?(jdemooij) → review+
Attachment #8541762 - Flags: review?(jdemooij) → review+
Attachment #8541770 - Flags: review?(jdemooij) → review+
Comment on attachment 8541779 [details] [diff] [review]
assembler-syntax.patch

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

Nice patches, BaseAssembler is getting quite pretty :)

::: js/src/jit/shared/BaseAssembler-x86-shared.h
@@ +4380,5 @@
> +            if (IsXMMReversedOperands(opcode)) {
> +                spew("%-11s%s, .Lfrom%d%+d(%%rip)", legacySSEOpName(name), nameFPReg(dst), label.offset(), ripOffset);
> +            } else {
> +                spew("%-11s.Lfrom%d%+d(%%rip), %s", legacySSEOpName(name), label.offset(), ripOffset, nameFPReg(dst));
> +            }

Pre-existing nit: looks like we can drop the {} here since both 'then' and 'else' blocks fit on one line.

@@ +4395,1 @@
>              }

Same here.
Attachment #8541779 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #8)
> @@ +133,5 @@
> > +        static const char* const r8names_norex[16]
> > +          = { "%al", "%cl", "%dl", "%bl", "%ah", "%ch", "%dh", "%bh",
> > +              "%r8b", "%r9b", "%r10b", "%r11b", "%r12b", "%r13b", "%r14b", "%r15b" };
> > +        int off = (RegisterID)reg - (RegisterID)eax;
> > +        return (off < 0 || off > 15) ? "%r???" : r8names_norex[off];
> 
> This matches the other method, but could we assert `off` is in range here
> and in nameFPReg / nameIReg? Or will it break something?

Good suggestion. I've now added asserts. I suspect the r??? stuff was added because it's annoying to be debugging one problem and then find that enabling debug spew causes crashes of its own. However, with all the fixes in the spew strings lately, I think it's now reasonably safe.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7fcb0691f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d710a4836ac9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea34b180725
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7fd289fa6fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/165bbe7a3759
https://hg.mozilla.org/integration/mozilla-inbound/rev/00b8d104bf31
https://hg.mozilla.org/integration/mozilla-inbound/rev/fefa97636279
Depends on: 1116367
You need to log in before you can comment on or make changes to this bug.