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

Jump to bottom

cmd/compile: plan9-amd64-9front builder failing in TestFma #35063

Closed
fhs opened this issue Oct 22, 2019 · 12 comments
Closed

cmd/compile: plan9-amd64-9front builder failing in TestFma #35063

fhs opened this issue Oct 22, 2019 · 12 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Plan9
Milestone
Go1.14

Comments

@fhs
Copy link
Contributor

fhs commented Oct 22, 2019 •
edited

Since CL 137156
(https://build.golang.org/log/9c5e9a06aa97d909dc1ef2e2edca437e7a320e7e) the builder has been failing in TestFma in math package:

fatal error: sys: trap: #UD pc=0x308886
[signal sys: trap: code=0x0 addr=0x0 pc=0x308886]

goroutine 67 [running]:
runtime.throw(0x4012000, 0x1a)
	/tmp/workdir-gnot/go/src/runtime/panic.go:774 +0x81 fp=0x402ee10 sp=0x402ede0 pc=0x22d3c1
runtime.sigpanic()
	/tmp/workdir-gnot/go/src/runtime/os_plan9.go:104 +0x367 fp=0x402ee80 sp=0x402ee10 pc=0x229c57
math_test.TestFma(0x4095d00)
	/tmp/workdir-gnot/go/src/math/all_test.go:3058 +0x96 fp=0x402ef70 sp=0x402ee80 pc=0x308886
testing.tRunner(0x4095d00, 0x360698)
	/tmp/workdir-gnot/go/src/testing/testing.go:909 +0xce fp=0x402efd0 sp=0x402ef70 pc=0x2babde
runtime.goexit()
	/tmp/workdir-gnot/go/src/runtime/asm_amd64.s:1375 +0x1 fp=0x402efd8 sp=0x402efd0 pc=0x257431
created by testing.(*T).Run
	/tmp/workdir-gnot/go/src/testing/testing.go:960 +0x363

goroutine 1 [chan receive]:
testing.(*T).Run(0x4095d00, 0x357374, 0x7, 0x360698, 0x26b201)
	/tmp/workdir-gnot/go/src/testing/testing.go:961 +0x38a
testing.runTests.func1(0x4084000)
	/tmp/workdir-gnot/go/src/testing/testing.go:1202 +0x7d
testing.tRunner(0x4084000, 0x4030db0)
	/tmp/workdir-gnot/go/src/testing/testing.go:909 +0xce
testing.runTests(0x4016060, 0x61c720, 0x47, 0x47, 0x0)
	/tmp/workdir-gnot/go/src/testing/testing.go:1200 +0x2c9
testing.(*M).Run(0x4080000, 0x0)
	/tmp/workdir-gnot/go/src/testing/testing.go:1117 +0x17b
main.main()
	_testmain.go:394 +0x13a
FAIL	math	0.441s
FAIL

CC @0intro
@gopherbot please add label OS-Plan9, NeedsInvestigation
@gopherbot gopherbot added OS-Plan9 NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 22, 2019
@agnivade
Copy link
Contributor

@smasher164

@agnivade agnivade added this to the Go1.14 milestone Oct 22, 2019
@smasher164 smasher164 self-assigned this Oct 22, 2019
@smasher164
Copy link
Member

smasher164 commented Oct 22, 2019 •
edited

A floating-point panic occurs on a call to the Fma function because of the following instruction sequence:

all_test.go:3058    0x3089b6        803d571a330000      CMPB $0x0, runtime.x86HasFMA(SB)    
all_test.go:3058    0x3089bd        0f84d8010000        JE 0x308b9b             
all_test.go:3058    0x3089c3        0f10e2              MOVUPS X2, X4               
all_test.go:3058    0x3089c6        c4e2e1b9d0660f2e    MOVL $0x2e0f66d0, CX            
bits.go:39          0x3089ce        d2                  ?

The comparison of runtime.x86HasFMA with 0 returns false, indicating that the instruction is present. The instruction at 0x3089c6 is reached, where c4e2e1b9d0660f2e and d2 actually decode as

vfmadd231sd %xmm0,%xmm3,%xmm2
ucomisd     %xmm2,%xmm2

The floating-point panic is a result of executing the vfmadd231sd instruction. However, I am not familiar enough with Plan 9 (9front) to know why the instruction doesn't execute. Is amd64 feature detection transparently returning true on the builder even if the instruction is not available? Or is there some restriction present in the 9front kernel? #15234 may be related.

@smasher164 smasher164 removed their assignment Oct 22, 2019
@0intro
Copy link
Member

0intro commented Oct 22, 2019

We usually disable SSE instructions on Plan 9 because FP aren't allowed in note handler. I think we should disable FMA on Plan 9.

@smasher164
Copy link
Member

I think we should disable FMA on Plan 9.

Sounds good to me. I'll send a CL shortly.

@0intro
Copy link
Member

0intro commented Oct 22, 2019

Look at how useSSE is done in src/cmd/compile/internal/ssa. Maybe we can add a useFMA configuration parameter.

@smasher164
Copy link
Member

Given that the x86HasFMA is already set up in src/runtime/proc.go as

x86HasFMA = cpu.X86.HasFMA

Can't I just change it to

x86HasFMA = cpu.X86.HasFMA && GOOS != "plan9"

This will cause feature detection to fail and default to the software fallback.

@0intro
Copy link
Member

0intro commented Oct 22, 2019

I think it's better to keep the compiler configuration separate from the CPU feature detection. That's what we did for SSE.

@0intro
Copy link
Member

0intro commented Oct 22, 2019

I think you can send a CL with your initial idea.

@smasher164
Copy link
Member

I sent CL https://go-review.googlesource.com/c/go/+/202617 which introduces a UseFMA flag to ssa.Config, and checks it in addF to call the software fallback on AMD64.

@gopherbot
Copy link

Change https://golang.org/cl/202617 mentions this issue: cmd/compile: don't use FMA on plan9/amd64

@smasher164
Copy link
Member

smasher164 commented Oct 22, 2019 •
edited

@0intro Is the plan9-amd64-9front builder down? I don't see it on the dashboard, and the trybot has been stuck on wait_static_builder.

@0intro
Copy link
Member

0intro commented Oct 22, 2019

@smasher164 Sorry, the builder is back online.

@golang golang locked and limited conversation to collaborators Oct 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Plan9
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
5 participants
@fhs @agnivade @0intro @gopherbot @smasher164