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: how to test? #25724

Open
robpike opened this issue Jun 4, 2018 · 9 comments
Open

cmd/asm: how to test? #25724

robpike opened this issue Jun 4, 2018 · 9 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@robpike
Copy link
Contributor

robpike commented Jun 4, 2018

The current tests are an inconsistent mess. Some files are hand-written. Others are auto-generated and enormous, and could be generated on the fly instead of being checked in. Some architectures are well tested. Others are not. Some subsets of architectures (AVX) are exhaustively tested. Others are not.

I would like a proper discussion about how to test the assembler (and the compiler, if you like). Without a clear picture, the tests will become enormous, wildly inconsistent, messy, and difficult to maintain.

@FiloSottile FiloSottile added the Testing An issue that has been verified to require only test changes, not just a test failure. label Jun 5, 2018
@FiloSottile FiloSottile added this to the Go1.12 milestone Jun 5, 2018
@quasilyte
Copy link
Contributor

quasilyte commented Jun 5, 2018

Thank you for opening an issue.

Please, correct me where I'm wrong.

(quotes from https://go-review.googlesource.com/c/go/+/115858)

From @ianlancetaylor :

I think that Rob is saying that we previously did not have machine generated tests for the assembler. Now, it appears, we do.

We had x86test program written by Russ that generated amd64enc.s test suite.
It covered most legacy and some early AVX instructions

Many of those were commented-out with TODO comments, I've implemented missing instructions and performed uncommenting automatically with x86avxgen program (for legacy instruction, this was done by hand).

From @robpike:

Why does AVX get so much attention?

Because legacy x86 instruction already had coverage in amd64enc.s.
The second reason is because it was my objective to implement missing early AVX extensions as well as AVX-512.

Some architectures are well tested. Others are not

That may sound unfair, but if I'm working on x86 arch, it makes sense that I care more about that one.
At least I expect not to be blamed that other architectures not have auto generated suites right now.

To conclude: x86 test suite has auto generated tests for both new and older instructions.
The hand-written + auto generated test suites are 2 approaches Rob suggested in referenced CL, I've used in combination to reduce amount of auto-generated tests to be emitted and they helped a lot during implementation.

Also I didn't say, but will say now, I would prefer that such massive test files, such as this one and the ones in 115858, should be generated during the test rather than checked in.

I agree on this one, but when I've joined the project, it already had big auto-generated asm test file. I though it's logical and consistent to add another one to make it cover new instructions.
That's it.

@quasilyte
Copy link
Contributor

Also, I want to emphasize:
If AVX-512 ISA is extended (and it almost certainly will), new instructions can be implemented as easily as running x86avxgen again. And test generator is ready for it.
This was initial goal.

@quasilyte
Copy link
Contributor

CC @TocarIP

@quasilyte
Copy link
Contributor

I'll describe current situation to make this discussion easier. Just in case any details are missing from my previous responses.

I would like a proper discussion about how to test the assembler (and the compiler, if you like). Without a clear picture, the tests will become enormous, wildly inconsistent, messy, and difficult to maintain.

With x86, it was "almost" exhaustive auto-generated tests for both legacy and AVX instructions.
amd64enc.s had some forms uncovered due to different reasons, one of them is special cases for some instructions. When errors in asm were discovered, tests were added to amd64enc_extra, to avoid modifications to auto-generated test suite. With AVX-512 I think auto-generated tests cover all angles, except opcode suffixes. If we are to generate opcode suffixes for each form, it will increase amount of auto generated tests by a measurable margin, while adding all combinations to amd64enc only takes around 10 lines.

Hand-written tests also catch regression bugs.
These can be probably be separated.
They are annoying to support in auto-generated tests because most of these tests have backwards-compatibility reasons that are not reflected in ISA/encoding or any other technical aspects. The're just there.

This is the current approach, we can discuss alternatives and improvements now, if the situation is clear.

@mmcloughlin
Copy link
Contributor

Where is the code for avx512test? It's referenced in all the AVX-512 testdata files but I can't seem to find it.

// Code generated by avx512test. DO NOT EDIT.

Likewise, was x86test was ever landed? https://golang.org/cl/18842

I don't personally see the need to generate these test files on the fly. I tend to have a preference for keeping the files in the repo, but with a CI job that confirms they are in sync with the generator (for example run go generate ./... and confirm no changes to the git repo). This also has the benefit of preventing hand edits.

However I think it is is problematic if the programs that generated them are unavailable or out of sync.

I don't have the same perspective as others here, so apologies if I'm speaking out of turn. It seems to me it would be great to get to the point where all x86 generated files in the Go repo can be reproduced from tools in golang.org/x/arch/x86. Looks like this might require:

  • Landing x86testin some form
  • Landing avx512test
  • If there are any hand-written test cases mixed in, extracting them into separate files
  • I think different versions of x86avxgen were used to generate testdata files: c319b3c35c and b19384d3c1? Maybe we would need to resurrect the old one?

Ideally, some form of https://golang.org/cl/104496 could be landed as well. The goal would be that all these code generators take their input from the same (canonical) database of Go x86 instructions.

Thoughts? I'm interested in contributing in these areas if I can be of use :)

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 28, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Nov 28, 2018
@pwaller
Copy link
Contributor

pwaller commented Apr 10, 2019

Ping @quasilyte - any chance of an update in response to @mmcloughlin? A few people are interested in picking up where you left off but it's unclear what the final state of things was.

@quasilyte
Copy link
Contributor

Thanks for the ping. I thought I answered @mmcloughlin, but seems I was distracted and completely forgot about it.
I'll open source avx512test in a few days.

@mmcloughlin
Copy link
Contributor

Great, thanks @quasilyte! From my point of view it doesn't need to be clean at all, just throw up whatever you have on your machine :) The initial goal is just to get to the point where we can reproduce all these generated files, then we can consider any cleanup/refactor.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/170863 mentions this issue: cmd/gopherbot: CC triaged issues to owners

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

7 participants