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.

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

cmd/internal/obj: wrong code generated by the arm assembler #19141

Closed
benshi001 opened this issue Feb 17, 2017 · 13 comments
Closed

cmd/internal/obj: wrong code generated by the arm assembler #19141

benshi001 opened this issue Feb 17, 2017 · 13 comments

Comments

@benshi001
Copy link
Member

@benshi001 benshi001 commented Feb 17, 2017

The MULA is wrongly assembled by the ARM assembler, even the newest go.

For example, MULA r1, r2, r3, r4 is assembled to e0 23 41 92, which should be e0 24 31 92.

The addend is r3 and the result is r4, the assembler confuses them.

@benshi001
Copy link
Member Author

@benshi001 benshi001 commented Feb 17, 2017

While MULAWT is right,

MULAWT -> e1 24 31 c2

@benshi001
Copy link
Member Author

@benshi001 benshi001 commented Feb 17, 2017

I found this issue when tried to add test cases for my CL https://go-review.googlesource.com/#/c/35565/

@benshi001
Copy link
Member Author

@benshi001 benshi001 commented Feb 17, 2017

I have fixed this issue in patch set 15 of https://go-review.googlesource.com/#/c/35565/

The compiler also needs modification, since it followed the assembler's error in MULA.

@ALTree ALTree changed the title wrong code generated by the arm assembler cmd/internal/obj: wrong code generated by the arm assembler Feb 17, 2017
@ALTree ALTree added the NeedsFix label Mar 2, 2017
@ALTree ALTree added this to the Go1.9 milestone Mar 2, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 2, 2017

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

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 2, 2017

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

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Mar 22, 2017

The addend is r3 and the result is r4, the assembler confuses them.

It would be good and consistent that the last register is the destination. But I don't think this is documented and required. MULA has been there for a long time and there are probably user assembly code that rely on this encoding. Maybe just leave it as is?

@benshi001
Copy link
Member Author

@benshi001 benshi001 commented Mar 23, 2017

But MULAWB MULAWT use the last register as destination. That would confuse the user.

Maybe we would add other MULA serial instructions, like

Multiply and Subtract MLS 32 = 32 – 32 × 32
Signed Multiply Accumulate (halfwords) SMLABB 32 = 32 + 16 × 16
Signed Most Significant Word Multiply Accumulate SMMLA 32 = 32 + 32 × 32

How about these ones? Use the first as destination like MULA or use the last like MULAWB / MULAWT?

@benshi001
Copy link
Member Author

@benshi001 benshi001 commented Mar 27, 2017

Now I also agree with Cherry that we keep current circumstance unchanged.

Since a change in the assembler will cause
either changes in the compiler
or changes in the disassembler (otherwise test of MULA in cmd/asm/internal/asm/testdata/arm.s will fail)

For others MULS, MMULA, MMULS, MULABB, MULAWT, MULAWB, they can follow MULA, use the third register as destination and the fourth as the addend.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Mar 27, 2017

No, I didn't mean that we should change everything to the "wrong" MULA encoding.

MULAWT, MULAWB are in the "correct" encoding, which uses last register as destination. They should keep unchanged. The new instructions should follow the correct encoding.

I think we'd want MULA fixed. But it needs to be done more carefully, since there might be user code that uses it.

@benshi001
Copy link
Member Author

@benshi001 benshi001 commented Mar 27, 2017

@benshi001
Copy link
Member Author

@benshi001 benshi001 commented Mar 28, 2017

Now CL https://go-review.googlesource.com/#/c/35565/ has nothing to do with this issue. Since I
removed all changes to MULA/MULAWT/MULAWB, and only make the new MULA like instructions follow MULAWT in the right form.

And I will create a new CL to fix this issue.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 28, 2017

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

@benshi001
Copy link
Member Author

@benshi001 benshi001 commented Apr 28, 2017

@gopherbot gopherbot closed this in 4b2f7b4 May 6, 2017
@golang golang locked and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants