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: ppc64 builds failing with ssa checker on #36723

Closed
josharian opened this issue Jan 24, 2020 · 9 comments
Closed

cmd/compile: ppc64 builds failing with ssa checker on #36723

josharian opened this issue Jan 24, 2020 · 9 comments
Labels
Milestone

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Jan 24, 2020

At tip now:

$ GOOS=linux GOARCH=ppc64 go build -gcflags="all=-d=ssa/check/on" -a std
# runtime
compile: invalid offset for DS form load/store 00096 (/Users/josh/go/tip/src/runtime/symtab.go:821)	MOVW	1(R3), R3

Oddly, that error is coming from the assembler. Yet it doesn't occur with the flag off. Running with the SSA checker on shouldn't impact the generated code (I thought?).

At 1.13 we get different failures:

$ GOOS=linux GOARCH=ppc64 go build -gcflags="all=-d=ssa/check/on" -a std
# math
math/sqrt.go:102:14: internal compiler error: 'sqrt': val v149 is in reg but not live at end of b26
# runtime
runtime/malloc.go:414:18: internal compiler error: 'mallocinit': val v3 is in reg but not live at end of b15

Tentatively marking as Go 1.14 until we know whether we're actually generating bad code.

cc @dr2chase @randall77

@josharian
Copy link
Contributor Author

@josharian josharian commented Jan 24, 2020

Also, why isn't the ssa check builder isn't failing with this? Maybe... umm.. @dmitshur knows?

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jan 24, 2020

I think the SSA check builder only runs for AMD64. It doesn't do cross compilations.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jan 24, 2020

Running with the SSA checker on shouldn't impact the generated code (I thought?).

I also thought so. But then I found this
https://go.googlesource.com/go/+/refs/heads/master/src/cmd/compile/internal/ssa/compile.go#80

My guess is that it might catch a real bug that we (accidentally) depended on value ordering.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 24, 2020

Change https://golang.org/cl/216379 mentions this issue: cmd/compile: on PPC64, fold offset into some loads/stores only when offset is 4-aligned

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jan 24, 2020

CL https://go-review.googlesource.com/c/go/+/216379 should fix the immediate problem.

When SSA check is on, the Values are reordered, which affects the ordering of rewriting rules firing (and eventually whether some rules are fired or not), which affects the compilation result.

@josharian
Copy link
Contributor Author

@josharian josharian commented Jan 24, 2020

Thanks, @cherrymui! (Although it's going to cause me a bunch of rebase pain. No good deed goes unpunished. :P)

We should probably find some way to run with SSA check on for all architectures, somewhere.

And IMHO, it'd be good to use different random seeds for different runs, to get better coverage. I now remember having that discussion with Keith when the CL first went by.

@josharian
Copy link
Contributor Author

@josharian josharian commented Jan 24, 2020

Filed #36756 for the builder and sent https://go-review.googlesource.com/c/go/+/216418 for the seed randomization.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jan 26, 2020

Tests in cmd/compile/internal/gc/ssa_test.go are run with SSA check enabled (on each host architecture). It covers basic operations but is not as complete as building std.

@ceseo
Copy link
Contributor

@ceseo ceseo commented Jan 27, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.