Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/asm: s390x tests are suspicious #18295

Closed
rsc opened this issue Dec 13, 2016 · 3 comments
Closed

cmd/asm: s390x tests are suspicious #18295

rsc opened this issue Dec 13, 2016 · 3 comments
Labels
FrozenDueToAge
Milestone
Go1.9

Comments

@rsc
Copy link
Contributor

rsc commented Dec 13, 2016

cmd/asm/internal/asm/testdata/s390x.s says:

    // Some vector instructions have their inputs reordered.
    // Typically the reordering puts the length/index input into From3.
    VGEG    $1, 8(R15)(V30*1), V31  // VGEG    8(R15)(V30*1), $1, V31  // e7fef0081c12
    VSCEG   $1, V31, 16(R15)(V30*1) // VSCEG   V31, $1, 16(R15)(V30*1) // e7fef0101c1a
    VGEF    $0, 2048(R15)(V1*1), V2 // VGEF    2048(R15)(V1*1), $0, V2 // e721f8000013
    VSCEF   $0, V2, 4095(R15)(V1*1) // VSCEF   V2, $0, 4095(R15)(V1*1) // e721ffff001b
    VLL     R0, (R15), V1           // VLL     (R15), R0, V1           // e710f0000037
    VSTL    R0, V16, (R15)          // VSTL    V16, R0, (R15)          // e700f000083f
    VGMH    $8, $16, V12            // VGMH    $16, $8, V12            // e7c008101046
    VLEIF   $2, $-43, V16           // VLEIF   $-43, $2, V16           // e700ffd52843
    VSLDB   $3, V1, V16, V18        // VSLDB   V1, V16, $3, V18        // e72100030a77
    VERIMB  $2, V31, V1, V2         // VERIMB  V31, V1, $2, V2         // e72f10020472
    VSEL    V1, V2, V3, V4          // VSEL    V2, V3, V1, V4          // e7412000308d

This seems wrong. The first column is the assembler input. The second column is the result of parsing column 1 and then printing it back. The second column is supposed to be semantically identical to the first, different only if the assembler printing uses a different spelling than the input. For example if you wrote 'MOV $0x00, R1' then it might reasonably print instead as 'MOV $0, R1'. But clearly the two mean the same thing as asssembler input.

In contrast, it is far from clear to me that 'VSEL V1, V2, V3, V4' and 'VSEL V2, V3, V1, V4' mean the same thing as assembler input. In fact I'd be very surprised if they did.

At this point probably the input meanings cannot be changed, but the printer should be fixed.

Not blocking Go 1.8.

/cc @mundaym @randall77
@rsc rsc added this to the Go1.9 milestone Dec 13, 2016
@mundaym
Copy link
Member

mundaym commented Dec 13, 2016 •
edited

Fixing the printing sounds good to me. We could also modify the order that asmz.go expects, avoiding the re-ordering entirely. (This would entail placing the first operand into From, the second into Reg, the third into From3 and the fourth into To. As opposed to re-ordering them to put the source operand into From.)

Some background:

This ordering stems from the implementation of the storage-to-storage instructions. For example, MVC $8, (R1), (R2) which moves 8 bytes from (R1) to (R2). (In gas this would be written as mvc 0(8, %r2), 0(%r1)). To integrate nicely into the old back-end the storage-to-storage instructions needed to have the source memory operand in From and the destination memory operand in To. The length couldn't be placed in Reg so I ended up putting it into From3.

When implementing the vector instructions it seemed to make sense to stick to the same convention, placing the length/index (the first operand, if it exists) into From3, the source operand into From, (the second source operand into Reg if it exists,) and the destination operand into To. But of course this leads to inputs being re-ordered when they are printed.

There is no particular reason anymore to keep this ordering in the back-end (AFAIK From isn't treated specially anymore), so long as the order the user sees stays the same.

1.9 seems reasonable, since this only really impacts people writing s390x assembly (except for MVC instructions which are emitted by the compiler as MVC Rsrc, $len, Rdst).

@rsc
Copy link
Contributor Author

rsc commented Dec 13, 2016

Thanks for the background. I agree that fixing the printing seems best. (I'd rather not invalidate all the existing assembly.) Whether you fix the printing by a special case in the printing function or by changing the internal representation is up to you.

@gopherbot
Copy link

CL https://golang.org/cl/40299 mentions this issue.

@golang golang locked and limited conversation to collaborators Apr 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
3 participants
@rsc @gopherbot @mundaym