1070985 - IonMonkey x64: add some move instructions with a constant address operand, and fix the formatters to emit REX prefixes.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Douglas Crosher [:dougc]]

The x64 supports an immediate 32 bit constant in the addressing mode. There is already some support for move instructions with 32 bit constant operand addresses but these are only enabled on the x86 and the formatters for these did not set the REX prefix.

I would at least like to see the formatters fixed as I use these instructions in some experimental code and don't want other developers to waste time tracking down the subtle bugs that occur when the REX prefixes are not emitted - bugs show up under register pressure.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Add some move instructions with a constant address operand, and fix the formatters to emit REX prefixes.

Comment 2

[Douglas Crosher [:dougc]]

Comment on attachment 8493071 [details] [diff] [review]
Add some move instructions with a constant address operand, and fix the formatters to emit REX prefixes.

This patch enables a number of instruction for the x64 that were only compiled for the x86. These are all supported on the x64, but perhaps were disabled because they had not immediate use. When enabled for the x64 there were problems in the formatters due to missing REX prefixes.

I would at least like to get the REX prefixes fixed, but am open to not enabling the unused instructions for the x64 in which case I'll split the patch?

I use the enabled instruction in some experimental code.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8493071 [details] [diff] [review]
Add some move instructions with a constant address operand, and fix the formatters to emit REX prefixes.

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

::: js/src/jit/shared/BaseAssembler-x86-shared.h
@@ +4388,5 @@
> +#ifdef JS_CODEGEN_X86
> +            MOZ_ASSERT(!byteRegRequiresRex(reg));
> +#endif
> +            m_buffer.ensureSpace(maxInstructionSize);
> +            emitRexIf(byteRegRequiresRex(reg), reg, 0, 0);

nit: Use emitRexIfNeeded, and move the above assertion into emitRexIfNeeded, as well as doing it for all registers.

Comment 4

[Douglas Crosher [:dougc]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Comment on attachment 8493071 [details] [diff] [review]
> Add some move instructions with a constant address operand, and fix the
> formatters to emit REX prefixes.
> 
> Review of attachment 8493071 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/shared/BaseAssembler-x86-shared.h
> @@ +4388,5 @@
> > +#ifdef JS_CODEGEN_X86
> > +            MOZ_ASSERT(!byteRegRequiresRex(reg));
> > +#endif
> > +            m_buffer.ensureSpace(maxInstructionSize);
> > +            emitRexIf(byteRegRequiresRex(reg), reg, 0, 0);
> 
> nit: Use emitRexIfNeeded, and move the above assertion into emitRexIfNeeded,
> as well as doing it for all registers.

Thank you for the quick review. emitRexIfNeeded is used for word sized operands. In this case the 'reg' is a byte sized operand so it needs to use byteRegRequiresRex, see the comments for emitRexIf. It's because there are ah, and al, etc for byte sized operands, but only eax or rax for large sized operands. Thus the above code looks correct as-is, and can it be left as-is?

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Douglas Crosher [:dougc] from comment #4)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > Comment on attachment 8493071 [details] [diff] [review]
> > Add some move instructions with a constant address operand, and fix the
> > formatters to emit REX prefixes.
> > 
> > Review of attachment 8493071 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/jit/shared/BaseAssembler-x86-shared.h
> > @@ +4388,5 @@
> > > +#ifdef JS_CODEGEN_X86
> > > +            MOZ_ASSERT(!byteRegRequiresRex(reg));
> > > +#endif
> > > +            m_buffer.ensureSpace(maxInstructionSize);
> > > +            emitRexIf(byteRegRequiresRex(reg), reg, 0, 0);
> > 
> > nit: Use emitRexIfNeeded, and move the above assertion into emitRexIfNeeded,
> > as well as doing it for all registers.
> 
> Thank you for the quick review. emitRexIfNeeded is used for word sized
> operands. In this case the 'reg' is a byte sized operand so it needs to use
> byteRegRequiresRex, see the comments for emitRexIf. It's because there are
> ah, and al, etc for byte sized operands, but only eax or rax for large sized
> operands. Thus the above code looks correct as-is, and can it be left as-is?

Indeed, I guess I confused byteRegRequiresRex with regRequiresRex.  So, yes
.

Comment 6

[Douglas Crosher [:dougc]]

Attachment: Add some move instructions with a constant address operand, and fix the formatters to emit REX prefixes.

Rebase. Carrying forward r+.

Comment 7

[Douglas Crosher [:dougc]]

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

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f6b6ba01341