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/asm: doesn't support PCALIGN on i386/amd64 #56474

Closed
piotr-sneller opened this issue Oct 28, 2022 · 9 comments
Closed

cmd/asm: doesn't support PCALIGN on i386/amd64 #56474

piotr-sneller opened this issue Oct 28, 2022 · 9 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@piotr-sneller
Copy link

piotr-sneller commented Oct 28, 2022

What version of Go are you using (go version)?

go version go1.19.2 windows/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

Tried to compile the following assembly function:

TEXT test(SB), NOFRAME|NOSPLIT, $0-0
PCALIGN $2
RET

What did you expect to see?

I expected the function to compile correctly with the RET aligned to a 2-byte boundary.

What did you see instead?

asm: asmins: missing op 00000 (<unknown line number>)   PCALIGN $2
asm: assembly failed

Per my google research, it appears the directive PCALIGN is recognized by the assembler, but it is not implemented on the x86/x64 targets. Could you please implement this feature, as it is essential to high-performance assembly code? It should be working both with the TEXT (function/label alignment) and DATA (e.g. AVX512 64-byte constants) sections.

@heschi heschi changed the title affected/package: go/assembler cmd/asm: doesn't support PCALIGN on i386/amd64 Oct 28, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 28, 2022
@heschi
Copy link
Contributor

heschi commented Oct 28, 2022

cc @golang/compiler

@heschi heschi added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 28, 2022
@heschi heschi added this to the Backlog milestone Oct 28, 2022
@randall77
Copy link
Contributor

randall77 commented Oct 30, 2022

I believe that opcode is only implemented on arm64 and ppc64 so far.

It shouldn't be hard to do - we already pad on x86 for various alignment restrictions.

@piotr-sneller
Copy link
Author

I am not able to help here, as my proficiency in the Go internals is insufficient. I would be enormously grateful though if someone with the required skills decided to contribute the necessary patch.

@petr-sneller
Copy link

I have looked into it and I was able to actually add the support into span6() function (x86/amd64 backend), looking like this:

			if p.As == obj.APCALIGN {
				v := -c & int32(p.From.Offset-1)
				s.Grow(int64(c) + int64(v))
				fillnop(s.P[c:], int(v))
				c += v
				pPrev = p
				continue
			}

I'm just wondering, would this change be acceptable, should I try and submit a PR regarding this feature? How to test it?

I have personally no intention about modifying anything in the go language itself. I just think that it would be nice to be able to align stuff at asm level as it's just beneficial to have this kind of control be it hot loops or data for AVX2/AVX512. I think aligning everything makes no sense, only the stuff that you want or that improves based on profiling.

The problem I have at the moment is that if you have .s file that implements bunch of functions at asm level, you can change something that would regress something completely unrelated, because some labels shift their offsets and end up in a different cache lines, etc... The difference of this in our software could be up to 1 GB/s throughput on a 16 core machine, which is normally able to process around 37.5GB/s of data.

@randall77
Copy link
Contributor

Seems reasonable to me.

Testing is tricky. Normally we test non-semantics-changing things like this in test/codegen, but that doesn't have any assembly tests currently. Maybe something in cmd/asm/internal/asm? It would be worth at least having a test that runs code that needs padding, just to make sure the nops are correct.

Note that if you want alignments more than 32 bytes, just aligning within a function isn't enough. You'll need to increase the alignment of function starts, which will be trickier.

@petr-sneller
Copy link

Can the alignment of a function start be increased automatically when a PCALIGN is used within that function? I have seen the alignment info in FuncInfo, but I have no idea whether a greater number than 32 would be honored and actually when to update it so it gets honored.

@piotr-sneller
Copy link
Author

Code alignment up to 32 bytes would already be great progress, but data alignment to a 64-byte boundary is critical to AVX512 code. The lack of it essentially prohibits the use of VMOVDQA instruction family. This, in turn, halves the available data cache bandwidth, as the CPU needs to issue two memory operations under the hood and combine the results.

@cherrymui
Copy link
Member

You're welcome to send a patch. Thanks! If you have not, please see the contribution guide for how to contribute. https://go.dev/doc/contribute

For testing, there are similar tests for other architectures
https://cs.opensource.google/go/go/+/master:src/cmd/internal/obj/ppc64/asm_test.go;l=331
https://cs.opensource.google/go/go/+/master:src/cmd/internal/obj/arm64/asm_arm64_test.go;l=105
You probably can do something similar.

For alignment larger than the default function alignment, I think you can increase the function alignment like https://cs.opensource.google/go/go/+/master:src/cmd/internal/obj/arm64/asm7.go;l=1080

mauri870 added a commit to mauri870/go that referenced this issue Jul 22, 2023
The PCALIGN asm directive was not supported on i386/amd64,
causing a compile-time error when used. The same directive
is currently supported on arm64 and ppc64 arquitectures.

Fixes: golang#56474
mauri870 added a commit to mauri870/go that referenced this issue Jul 22, 2023
The PCALIGN asm directive was not supported on i386/amd64,
causing a compile-time error when used. The same directive
is currently supported on arm64 and ppc64 arquitectures.

Fixes: golang#56474
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/511662 mentions this issue: cmd/asm: add PCALIGN support for i386/amd64

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants