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

PCALIGN seems to be missing #419

Open
egonelbre opened this issue Jan 20, 2024 · 5 comments · May be fixed by #420
Open

PCALIGN seems to be missing #419

egonelbre opened this issue Jan 20, 2024 · 5 comments · May be fixed by #420
Labels
instructions Instruction database

Comments

@egonelbre
Copy link

Go will soon support PCALIGN pseudo instruction for amd64.

https://tip.golang.org/doc/asm#special-instructions

@mmcloughlin
Copy link
Owner

Thanks for the issue. Do you think it would be sufficient to just append this to the instruction set (probably with the opcodesextra mechanism)? Or is there some reason we'd need to handle these pseudo instructions differently?

@mmcloughlin mmcloughlin added the instructions Instruction database label Jan 20, 2024
@mmcloughlin
Copy link
Owner

mmcloughlin commented Jan 20, 2024

PCALIGN is handled by the architecture-independent part of the Go assembler:

https://github.com/golang/go/blob/go1.21.6/src/cmd/asm/internal/asm/asm.go#L336

There are other pseudo-ops defined here, for example FUNCDATA and PCDATA mentioned in #144.

At the moment I can't think of a reason not to just treat this as another ISA instruction. Of course it's still a goal for avo to support ARM #189, in which case these pseudo-ops would need to be pulled out into an architecture-independent set of instructions. But we can cross that bridge when we come to it.

@mmcloughlin mmcloughlin linked a pull request Jan 20, 2024 that will close this issue
@mmcloughlin
Copy link
Owner

Oh. I think I was thrown off by the fact that the PCALIGN instruction has existed in the assembler for a long time. But as you've said there wasn't backend support for it in amd64 until recently golang/go#56474.

PR #420 adds it but is failing assembler tests on Go 1.20 and 1.21 for this reason.

This is another use case for avo tracking the Go version required for each instruction #84.

@egonelbre
Copy link
Author

egonelbre commented Jan 20, 2024

You can use go1.22rc1 for testing that. As for the implementation, I don't have that much experience with avo to have an opinion on how to implement it.

For now I did this in my own code:

Instruction(&ir.Instruction{
	Opcode:   "PCALIGN",
	Operands: []Op{Imm(1024)},
})

@mmcloughlin
Copy link
Owner

You can use go1.22rc1 for testing that.

For sure. The problem is the test I have that validates the avo instruction database by assembling a generated file with one example of every instruction form. This test runs in CI with the installed Go version, so at the moment with Go 1.20 and 1.21. Therefore the tests fail when I add the PCALIGN instruction.

The fix would be to either install a pinned Go version for the purpose of that test. Or alternatively, implement some form of #84 and store the version required for each instruction, which would then allow me to limit which instructions appear in the assembler test file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
instructions Instruction database
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants