Re-enable vgather in tests when x64asm is fixed

[stefanheule]

No description provided.

[stefanheule]

Would one of you mind taking this over? I have an easy to reproduce version, but I don't really know how to go from there. Basically, the sanbox cannot run the following code:

  .text
  .globl target
  .type target, @function
.target:                               
  vgatherqpd %ymm11, 0x16(%r8), %ymm5
  retq                            # 6  

.size target, .-target

[eschkufz]

I think it must be that x64asm isn't assembling this correctly.

You can assign it to me but I won't be able to look at it for a bit.

On Tue, Feb 24, 2015 at 11:28 PM, Stefan Heule [email protected]
wrote:

Would one of you mind taking this over? I have an easy to reproduce
version, but I don't really know how to go from there. Basically, the
sanbox cannot run the following code:

.text
.globl target
.type target, @function
.target:
vgatherqpd %ymm11, 0x16(%r8), %ymm5
retq # 6

.size target, .-target

—
Reply to this email directly or view it on GitHub
#552 (comment).

[stefanheule]

Yeah, that's what I suspect, too, I just don't know how I can check that (easily). I guess if it's easy for you to give me a few quick hints how to check that, then I'll give it another try.

[eschkufz]

Take the one instruction and put it in a text file, foo.s.

Then from the command line:

$ g++ -c foo.s
$ objdump -d -Msuffix foo.o > gcc
$ cat foo.s | /bin/asm > x64asm

And compare the two files. That will at least show you how (if) the two
assemblers differ.

On Tue, Feb 24, 2015 at 11:30 PM, Stefan Heule [email protected]
wrote:

Yeah, that's what I suspect, too, I just don't know how I can check that
(easily). I guess if it's easy for you to give me a few quick hints how to
check that, then I'll give it another try.

—
Reply to this email directly or view it on GitHub
#552 (comment).

[stefanheule]

g++ is already unhappy: Error: invalid VSIB address for vgatherqpd

[eschkufz]

Okay. I understand this. Explanation coming --

On Tue, Feb 24, 2015 at 11:38 PM, Stefan Heule [email protected]
wrote:

g++ is already unhappy: Error: invalid VSIB address forvgatherqpd'`

—
Reply to this email directly or view it on GitHub
#552 (comment).

[eschkufz]

This is the same issue as the one that's already in x64asm. At least it's
related --

This one instruction takes a specific kind of memory operand. Instead of
the usual memory operand form, it takes an xmm or a ymm register as the
base. This is what corresponds to the mystery vm32x/vm32y/vm64x/vm64y types
in the spreadsheet.

What you'll need to do is --

1. Create two new memory types VM32 <: M, VM64 <: M.
2. Change Codegen.hs so that when it sees an entry these spreadsheets types
   it canonicalizes them to something reasonable:

canonical_op "vm32x" = "vm32"
canonical_op "vm32y" = "vm32"
canonical_op "vm64x" = "vm64"
canonical_op "vm64y" = "vm64"

1. Introduce new handlers for these types in Codegen.hs

op2type "m32" = "VM32"
op2type "m64" = "VM64"

op2tag "m32" = "VM_32"
op2tag "m64" = "VM_64"

1. Add new types to type.h

VM_32
VM_64

1. Add some new methods to Assembler for emitting a sib byte for these...
   This isn't easy to explain off the top of my head, but you can follow the
   compile errors you'll get and at least see where the new method needs to
   go. The actual implementation shouldn't be hard. It'll assemble exactly the
   way the SIB byte is normally assembled. Just make the analogy to an R
   argument.

Ug... let's cross this bridge when you get here

1. Add new handlers in Transforms for when you see an operand of this type.
   You'll basically introduce a new get_m() method that looks like the
   existing ones which samples the base register from xmm/ymm rather than
   r64/32 pools.
2. Congratulate yourself. You understand the nastiest parts of how the
   assembler works.

On Tue, Feb 24, 2015 at 11:38 PM, eric schkufza [email protected]
wrote:

Okay. I understand this. Explanation coming --

On Tue, Feb 24, 2015 at 11:38 PM, Stefan Heule [email protected]
wrote:

g++ is already unhappy: Error: invalid VSIB address forvgatherqpd'`

—
Reply to this email directly or view it on GitHub
#552 (comment).

[eschkufz]

Steps 1-5 correspond to fixing the ticket that's already in x64asm.

If you just add dummy handlers to Transforms for returning failure when you
see operand types of this kind, the fuzz tester bug should go away. If you
add sensible handlers we'll have actual support for this wretched
instruction.

On Tue, Feb 24, 2015 at 11:48 PM, eric schkufza [email protected]
wrote:

This is the same issue as the one that's already in x64asm. At least it's
related --

This one instruction takes a specific kind of memory operand. Instead of
the usual memory operand form, it takes an xmm or a ymm register as the
base. This is what corresponds to the mystery vm32x/vm32y/vm64x/vm64y types
in the spreadsheet.

What you'll need to do is --

1. Create two new memory types VM32 <: M, VM64 <: M.
2. Change Codegen.hs so that when it sees an entry these spreadsheets
   types it canonicalizes them to something reasonable:

canonical_op "vm32x" = "vm32"
canonical_op "vm32y" = "vm32"
canonical_op "vm64x" = "vm64"
canonical_op "vm64y" = "vm64"

1. Introduce new handlers for these types in Codegen.hs

op2type "m32" = "VM32"
op2type "m64" = "VM64"

op2tag "m32" = "VM_32"
op2tag "m64" = "VM_64"

1. Add new types to type.h

VM_32
VM_64

1. Add some new methods to Assembler for emitting a sib byte for these...
   This isn't easy to explain off the top of my head, but you can follow the
   compile errors you'll get and at least see where the new method needs to
   go. The actual implementation shouldn't be hard. It'll assemble exactly the
   way the SIB byte is normally assembled. Just make the analogy to an R
   argument.

Ug... let's cross this bridge when you get here

1. Add new handlers in Transforms for when you see an operand of this
   type. You'll basically introduce a new get_m() method that looks like the
   existing ones which samples the base register from xmm/ymm rather than
   r64/32 pools.
2. Congratulate yourself. You understand the nastiest parts of how the
   assembler works.

On Tue, Feb 24, 2015 at 11:38 PM, eric schkufza [email protected]
wrote:

Okay. I understand this. Explanation coming --

On Tue, Feb 24, 2015 at 11:38 PM, Stefan Heule [email protected]
wrote:

g++ is already unhappy: Error: invalid VSIB address forvgatherqpd'`

—
Reply to this email directly or view it on GitHub
#552 (comment).

[eschkufz]

I can say a little bit more about step 5 --

The method you're going to have to fix is Assembler::mod_rm_sib(). Wow.
Horrible.

This method is an implementation of table 2-3 from vol2 of the intel
manual. You'll need to create a new version of this method for the new
memory types. (Aren't you glad you understand how sfinae works now?)

The new version is going to have to implement table 2-13. The mod/rm byte
components of the methods should probably be identical. It's only the part
that generates the sib byte that's going to have to be different.

On Tue, Feb 24, 2015 at 11:50 PM, eric schkufza [email protected]
wrote:

Steps 1-5 correspond to fixing the ticket that's already in x64asm.

If you just add dummy handlers to Transforms for returning failure when
you see operand types of this kind, the fuzz tester bug should go away. If
you add sensible handlers we'll have actual support for this wretched
instruction.

On Tue, Feb 24, 2015 at 11:48 PM, eric schkufza [email protected]
wrote:

This is the same issue as the one that's already in x64asm. At least it's
related --

This one instruction takes a specific kind of memory operand. Instead of
the usual memory operand form, it takes an xmm or a ymm register as the
base. This is what corresponds to the mystery vm32x/vm32y/vm64x/vm64y types
in the spreadsheet.

What you'll need to do is --

1. Create two new memory types VM32 <: M, VM64 <: M.
2. Change Codegen.hs so that when it sees an entry these spreadsheets
   types it canonicalizes them to something reasonable:

canonical_op "vm32x" = "vm32"
canonical_op "vm32y" = "vm32"
canonical_op "vm64x" = "vm64"
canonical_op "vm64y" = "vm64"

1. Introduce new handlers for these types in Codegen.hs

op2type "m32" = "VM32"
op2type "m64" = "VM64"

op2tag "m32" = "VM_32"
op2tag "m64" = "VM_64"

1. Add new types to type.h

VM_32
VM_64

1. Add some new methods to Assembler for emitting a sib byte for these...
   This isn't easy to explain off the top of my head, but you can follow the
   compile errors you'll get and at least see where the new method needs to
   go. The actual implementation shouldn't be hard. It'll assemble exactly the
   way the SIB byte is normally assembled. Just make the analogy to an R
   argument.

Ug... let's cross this bridge when you get here

1. Add new handlers in Transforms for when you see an operand of this
   type. You'll basically introduce a new get_m() method that looks like the
   existing ones which samples the base register from xmm/ymm rather than
   r64/32 pools.
2. Congratulate yourself. You understand the nastiest parts of how the
   assembler works.

On Tue, Feb 24, 2015 at 11:38 PM, eric schkufza [email protected]
wrote:

Okay. I understand this. Explanation coming --

On Tue, Feb 24, 2015 at 11:38 PM, Stefan Heule <[email protected]

wrote:

g++ is already unhappy: Error: invalid VSIB address forvgatherqpd'`

—
Reply to this email directly or view it on GitHub
#552 (comment).

[eschkufz]

I say you're going to have to use sfinae because this is a templated method
and you'll need a new specialization for the new mem types. The last I
looked at this code, it wasn't templated. I think that's because berkeley
has done some work in here.

You might have to check with him about how all this templating works now.

On Tue, Feb 24, 2015 at 11:59 PM, eric schkufza [email protected]
wrote:

I can say a little bit more about step 5 --

The method you're going to have to fix is Assembler::mod_rm_sib(). Wow.
Horrible.

This method is an implementation of table 2-3 from vol2 of the intel
manual. You'll need to create a new version of this method for the new
memory types. (Aren't you glad you understand how sfinae works now?)

The new version is going to have to implement table 2-13. The mod/rm byte
components of the methods should probably be identical. It's only the part
that generates the sib byte that's going to have to be different.

On Tue, Feb 24, 2015 at 11:50 PM, eric schkufza [email protected]
wrote:

Steps 1-5 correspond to fixing the ticket that's already in x64asm.

If you just add dummy handlers to Transforms for returning failure when
you see operand types of this kind, the fuzz tester bug should go away. If
you add sensible handlers we'll have actual support for this wretched
instruction.

On Tue, Feb 24, 2015 at 11:48 PM, eric schkufza [email protected]
wrote:

This is the same issue as the one that's already in x64asm. At least
it's related --

This one instruction takes a specific kind of memory operand. Instead of
the usual memory operand form, it takes an xmm or a ymm register as the
base. This is what corresponds to the mystery vm32x/vm32y/vm64x/vm64y types
in the spreadsheet.

What you'll need to do is --

1. Create two new memory types VM32 <: M, VM64 <: M.
2. Change Codegen.hs so that when it sees an entry these spreadsheets
   types it canonicalizes them to something reasonable:

canonical_op "vm32x" = "vm32"
canonical_op "vm32y" = "vm32"
canonical_op "vm64x" = "vm64"
canonical_op "vm64y" = "vm64"

1. Introduce new handlers for these types in Codegen.hs

op2type "m32" = "VM32"
op2type "m64" = "VM64"

op2tag "m32" = "VM_32"
op2tag "m64" = "VM_64"

1. Add new types to type.h

VM_32
VM_64

1. Add some new methods to Assembler for emitting a sib byte for
   these... This isn't easy to explain off the top of my head, but you can
   follow the compile errors you'll get and at least see where the new method
   needs to go. The actual implementation shouldn't be hard. It'll assemble
   exactly the way the SIB byte is normally assembled. Just make the analogy
   to an R argument.

Ug... let's cross this bridge when you get here

1. Add new handlers in Transforms for when you see an operand of this
   type. You'll basically introduce a new get_m() method that looks like the
   existing ones which samples the base register from xmm/ymm rather than
   r64/32 pools.
2. Congratulate yourself. You understand the nastiest parts of how the
   assembler works.

On Tue, Feb 24, 2015 at 11:38 PM, eric schkufza <[email protected]

wrote:

Okay. I understand this. Explanation coming --

On Tue, Feb 24, 2015 at 11:38 PM, Stefan Heule <
[email protected]> wrote:

g++ is already unhappy: Error: invalid VSIB address forvgatherqpd'`

—
Reply to this email directly or view it on GitHub
#552 (comment).

[bchurchill]

Is there more we're doing here or can we close this?

[eschkufz]

I think this was taken care of in another ticket (the build failing part)
and there's an open ticket in x64asm to solve the real problem.

I'd say change the name to "add support for vgather once x64asm is fixed"

On Thu, Feb 26, 2015 at 11:21 PM, Berkeley Churchill <
[email protected]> wrote:

Is there more we're doing here or can we close this?

—
Reply to this email directly or view it on GitHub
#552 (comment).

[stefanheule]

See also: StanfordPL/x64asm#149