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/compile: in the codgen test harness, always test architecture flavours like GO386=387 #24658

Closed
ALTree opened this issue Apr 3, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@ALTree
Copy link
Member

commented Apr 3, 2018

Opening this for discussion, as requested by @bradfitz.

In the new codegen test harness, code generation tests on architectures that have multiple "flavours" (like 386, with GO386={387,sse2}), look like this:

func sqrt(x float64) float64 {
	// 386:"FSQRT|SQRTSD"   (387 or sse2)
	return math.Sqrt(x)
}

Basically we just OR in the regex the ops from all the possible flavours. The test harness will compile sqrt with GOARCH=386 set, and that will default to generating sse2 ops (the default for 386), unless the environment also happen to have GO386=387 set, and in that case 387 ops will be generated.

This is somewhat brittle and has broken builders in the past, because GO386=387 is usually not set when running the codegen tests locally (except on a 387 machine), and we don't have a 387 trybot; so we only notice 386-related mistakes when they break the 387 builder. A similar problem may arise with the arm flavours (we have three: GOARM={5,6,7}), although this hasn't happened in practice (yet).

One possible solution is to make the codegen test harness compile each test that contains a 386 tag two times: first with GO386 set to sse2, then to 387. This will allow us to reliably catch and correct sse2-centric assumption in the 386 test regexps, even by just running the codegen tests locally. The downside is that the codegen tests will be a little slower.

Opinions? @randall77 @cherrymui @rasky @josharian

@ALTree ALTree added this to the Go1.11 milestone Apr 3, 2018

@rasky

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

I don't mind running it twice (and three times for GOARM) - it's still fast enough after std has been built - but I wonder what happens when we will eventually hit a test that cannot be compiled / doesn't make sense for a specific combination. For instance a test might be checking a GOARM7-only optimization that triggers generation of an opcode; running the same test under older GOARM doesn't make sense and is probably difficult to even safely write with the "or" pattern without breaking the effectiveness of the test for GOARM7.

So my suggestion is to do the multi-compilation by default, and then let developers specify a specific pattern for a specific arch, such as 386/sse2:"FSQRTD" or arm/7:"CLZ", in which case only that version is run (and of course you can explicitly use both 386/sse2 and 386/387 if you want to).

@rasky

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

any comment on this?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

I like your proposed 386/sse2, 386/387, arm/7 syntax when one wants to be specific.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

Yes, that sounds fine. We'd just need to compile each file N times, one for each subarch specified (and presumably, one subarch would be the default if no / specifier was present. Or does no / mean it must match for every subarch?).

@rasky

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

(and presumably, one subarch would be the default if no / specifier was present. Or does no / mean it must match for every subarch?)

I would go with the latter.

@rasky rasky added NeedsFix and removed NeedsDecision labels Apr 10, 2018

@rasky rasky self-assigned this Apr 10, 2018

@ALTree

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2018

If the goal is to find sse2-centric assumptions in the tests early (i.e. when running them locally, instead of when the 387 builder breaks), we definitely need to check every flavour by default, unless / is used.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 15, 2018

Change https://golang.org/cl/107315 mentions this issue: test: run codegen tests on all supported architecture variants

@gopherbot gopherbot closed this in 284ba47 Apr 15, 2018

@golang golang locked and limited conversation to collaborators Apr 15, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.