[go] cmd/compile: use ABI0 for cgo_unsafe_args functions
Cherry Zhang (Gerrit)
Cherry Zhang has uploaded this change for review.
cmd/compile: use ABI0 for cgo_unsafe_args functions
cgo_unsafe_args paragma indicates that the function (or its
callee) uses address and offsets to dispatch arguments, which
currently using ABI0 frame layout. Pin them to ABI0.
With this, "GOEXPERIMENT=regabi,regabiargs go run hello.go" works
on Darwin/AMD64.
Change-Id: I3eadd5a3646a9de8fa681fa0a7f46e7cdc217d24
---
M src/cmd/compile/internal/ssagen/ssa.go
1 file changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go
index 9c1c493..22993fe 100644
--- a/src/cmd/compile/internal/ssagen/ssa.go
+++ b/src/cmd/compile/internal/ssagen/ssa.go
@@ -232,6 +232,12 @@
if fn == nil {
return abi1
}
+ if fn.Pragma&ir.CgoUnsafeArgs != 0 {
+ // CgoUnsafeArgs indicates the function (or its callee) uses
+ // offsets to dispatch arguments, which currently using ABI0
+ // frame layout. Pin it to ABI0.
+ return abi0
+ }
switch fn.ABI {
case obj.ABI0:
return abi0
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Patch set 1:Run-TryBot +1
Than McIntosh (Gerrit)
Attention is currently required from: Austin Clements, Cherry Zhang.
Patch set 1:Code-Review +2
Cherry Zhang (Gerrit)
Attention is currently required from: Austin Clements, Cherry Zhang.
Cherry Zhang uploaded patch set #2 to this change.
cmd/compile: use ABI0 for cgo_unsafe_args functions
cgo_unsafe_args paragma indicates that the function (or its
callee) uses address and offsets to dispatch arguments, which
currently using ABI0 frame layout. Pin them to ABI0.
With this, "GOEXPERIMENT=regabi,regabiargs go run hello.go" works
on Darwin/AMD64.
Change-Id: I3eadd5a3646a9de8fa681fa0a7f46e7cdc217d24
---
M src/cmd/compile/internal/amd64/ssa.go
M src/cmd/compile/internal/ssagen/abi.go
2 files changed, 13 insertions(+), 4 deletions(-)
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Austin Clements.
Patch set 2:Run-TryBot +1
1 comment:
Patchset:
TRY=linux-amd64-regabi
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Attention is currently required from: Cherry Zhang.
3 comments:
File src/cmd/compile/internal/amd64/ssa.go:
Patch Set #2, Line 828:
// XXX s.ABI != obj.ABIInternal can only happen for ABI wrappersAh, interesting... I know this condition currently protects several things in this file, and this kind of suggests to me that the condition needs to be different everywhere. It seems like it was just luck that we weren't hitting this path before in ABI0 functions and I don't think we want to depend on that anywhere else. Maybe everywhere we use X15 needs to be protected by both the current function's ABI and the experiment?
For cgo_unsafe_args functions it is actually Go
// function so X15 should already be zero.This is also interesting. I'm okay with thinking of cgo_unsafe_args functions as "no, really, these are ABI0 functions" even though they could theoretically be somewhere in-between. Mostly I worry about introducing a third case that's kind of ABI0 and kind of ABIInternal but doesn't get exercised that much, particularly if/when we start shifting ABIInternal further away from ABI0.
File src/cmd/compile/internal/ssagen/abi.go:
Maybe move the CgoUnsafeArgs check up here with the other fn.ABI assignment?
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Cherry Zhang.
Cherry Zhang uploaded patch set #3 to this change.
cmd/compile: use ABI0 for cgo_unsafe_args functions
cgo_unsafe_args paragma indicates that the function (or its
callee) uses address and offsets to dispatch arguments, which
currently using ABI0 frame layout. Pin them to ABI0.
With this, "GOEXPERIMENT=regabi,regabiargs go run hello.go" works
on Darwin/AMD64.
Change-Id: I3eadd5a3646a9de8fa681fa0a7f46e7cdc217d24
---
M src/cmd/compile/internal/amd64/ssa.go
M src/cmd/compile/internal/ssagen/abi.go
2 files changed, 9 insertions(+), 8 deletions(-)
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Austin Clements.
4 comments:
Patchset:
Still need to fix Windows failures...
File src/cmd/compile/internal/amd64/ssa.go:
Patch Set #2, Line 828:
// XXX s.ABI != obj.ABIInternal can only happen for ABI wrappersAh, interesting... […]
I don't think it was just luck. Before, ABI0 functions being compiled are just wrappers, which have a very specific structure and I don't think they contain zeroing ops. That's why I wrote the condition.
We do need to do this for DUFFZERO also.
For cgo_unsafe_args functions it is actually Go
// function so X15 should already be zero.
This is also interesting. […]
Okay, just make them ABI0 functions, then. Dropped the comment.
File src/cmd/compile/internal/ssagen/abi.go:
Maybe move the CgoUnsafeArgs check up here with the other fn. […]
Done
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Austin Clements.
Patch set 3:Run-TryBot +1
Austin Clements (Gerrit)
Attention is currently required from: Cherry Zhang.
2 comments:
Patchset:
LGTM modulo Windows failures.
File src/cmd/compile/internal/amd64/ssa.go:
Patch Set #3, Line 1001:
if objabi.Experiment.RegabiG {Should we also guard this with s.ABI so this can appear in ABI0 functions? Or do you think we should just eliminate LoweredGetG when we commit to regabig?
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Cherry Zhang.
Cherry Zhang uploaded patch set #4 to this change.
cmd/compile: use ABI0 for cgo_unsafe_args functions
cgo_unsafe_args paragma indicates that the function (or its
callee) uses address and offsets to dispatch arguments, which
currently using ABI0 frame layout. Pin them to ABI0.
With this, "GOEXPERIMENT=regabi,regabiargs go run hello.go" works
on Darwin/AMD64.
Change-Id: I3eadd5a3646a9de8fa681fa0a7f46e7cdc217d24
---
M src/cmd/compile/internal/amd64/ssa.go
M src/cmd/compile/internal/ssa/gen/AMD64.rules
M src/cmd/compile/internal/ssa/rewriteAMD64.go
M src/cmd/compile/internal/ssagen/abi.go
4 files changed, 14 insertions(+), 12 deletions(-)
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Austin Clements.
1 comment:
File src/cmd/compile/internal/amd64/ssa.go:
Patch Set #3, Line 1001:
if objabi.Experiment.RegabiG {
Should we also guard this with s. […]
Good point. I guess we should guard this, and also the rewriting rule that lowers GetG. Done.
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Austin Clements.
Patch set 5:Run-TryBot +1
Cherry Zhang (Gerrit)
Attention is currently required from: Austin Clements.
1 comment:
Patchset:
LGTM modulo Windows failures.
The Windows failure is interesting. syscall.loadlibrary is a function defined in runtime and linkname'd to the syscall package. It is also cgo_unsafe_args. With this CL, in the runtime it gets an ABI0 TEXT symbol. In the syscall package, there is a Go declaration (bodyless), Go reference, and a dummy assembly reference. Somehow this triggers generation of ABI alias, so it gets an ABI0 ABIALIAS symbol. The two ABI0 symbols confuses the linker.
CL 306709 removes the dummy assembly reference, and it makes it work. (I don't see why we need the dummy reference.)
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Attention is currently required from: Cherry Zhang.
Patch set 5:Code-Review +2
1 comment:
File src/cmd/compile/internal/amd64/ssa.go:
Patch Set #5, Line 1002:
new ABIWhile we're here, "new ABI" -> "ABIInternal"? (It's "new" today but won't be in a few years :)
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Cherry Zhang.
Cherry Zhang uploaded patch set #6 to this change.
cmd/compile: use ABI0 for cgo_unsafe_args functions
cgo_unsafe_args paragma indicates that the function (or its
callee) uses address and offsets to dispatch arguments, which
currently using ABI0 frame layout. Pin them to ABI0.
With this, "GOEXPERIMENT=regabi,regabiargs go run hello.go" works
on Darwin/AMD64.
Change-Id: I3eadd5a3646a9de8fa681fa0a7f46e7cdc217d24
---
M src/cmd/compile/internal/amd64/ssa.go
M src/cmd/compile/internal/ssa/gen/AMD64.rules
M src/cmd/compile/internal/ssa/rewriteAMD64.go
M src/cmd/compile/internal/ssagen/abi.go
4 files changed, 15 insertions(+), 13 deletions(-)
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
1 comment:
File src/cmd/compile/internal/amd64/ssa.go:
Patch Set #5, Line 1002:
new ABIWhile we're here, "new ABI" -> "ABIInternal"? (It's "new" today but won't be in a few years :)
Done
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Cherry Zhang submitted this change.
cmd/compile: use ABI0 for cgo_unsafe_args functions
cgo_unsafe_args paragma indicates that the function (or its
callee) uses address and offsets to dispatch arguments, which
currently using ABI0 frame layout. Pin them to ABI0.
With this, "GOEXPERIMENT=regabi,regabiargs go run hello.go" works
on Darwin/AMD64.
Change-Id: I3eadd5a3646a9de8fa681fa0a7f46e7cdc217d24
Reviewed-on: https://go-review.googlesource.com/c/go/+/306609
Trust: Cherry Zhang <cher...@google.com>
Reviewed-by: Austin Clements <aus...@google.com>
Reviewed-by: Than McIntosh <th...@google.com>
---
M src/cmd/compile/internal/amd64/ssa.go
M src/cmd/compile/internal/ssa/gen/AMD64.rules
M src/cmd/compile/internal/ssa/rewriteAMD64.go
M src/cmd/compile/internal/ssagen/abi.go
4 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/src/cmd/compile/internal/amd64/ssa.go b/src/cmd/compile/internal/amd64/ssa.go
index 2c767d3..8142ba7 100644
--- a/src/cmd/compile/internal/amd64/ssa.go
+++ b/src/cmd/compile/internal/amd64/ssa.go
@@ -824,10 +824,7 @@
p.To.Reg = v.Args[0].Reg()
ssagen.AddAux2(&p.To, v, sc.Off64())
case ssa.OpAMD64MOVOstorezero:
- if s.ABI != obj.ABIInternal {
- v.Fatalf("MOVOstorezero can be only used in ABIInternal functions")
- }
- if !objabi.Experiment.RegabiG {
+ if !objabi.Experiment.RegabiG || s.ABI != obj.ABIInternal {
// zero X15 manually
opregreg(s, x86.AXORPS, x86.REG_X15, x86.REG_X15)
}
@@ -918,10 +915,7 @@
p.To.Type = obj.TYPE_REG
p.To.Reg = v.Reg()
case ssa.OpAMD64DUFFZERO:
- if s.ABI != obj.ABIInternal {
- v.Fatalf("MOVOconst can be only used in ABIInternal functions")
- }
- if !objabi.Experiment.RegabiG {
+ if !objabi.Experiment.RegabiG || s.ABI != obj.ABIInternal {
// zero X15 manually
opregreg(s, x86.AXORPS, x86.REG_X15, x86.REG_X15)
}
@@ -1004,8 +998,8 @@
// Closure pointer is DX.
ssagen.CheckLoweredGetClosurePtr(v)
case ssa.OpAMD64LoweredGetG:
- if objabi.Experiment.RegabiG {
- v.Fatalf("LoweredGetG should not appear in new ABI")
+ if objabi.Experiment.RegabiG && s.ABI == obj.ABIInternal {
+ v.Fatalf("LoweredGetG should not appear in ABIInternal")
}
r := v.Reg()
getgFromTLS(s, r)
diff --git a/src/cmd/compile/internal/ssa/gen/AMD64.rules b/src/cmd/compile/internal/ssa/gen/AMD64.rules
index 98cd865..839d4a3 100644
--- a/src/cmd/compile/internal/ssa/gen/AMD64.rules
+++ b/src/cmd/compile/internal/ssa/gen/AMD64.rules
@@ -460,7 +460,7 @@
(IsInBounds idx len) => (SETB (CMPQ idx len))
(IsSliceInBounds idx len) => (SETBE (CMPQ idx len))
(NilCheck ...) => (LoweredNilCheck ...)
-(GetG mem) && !objabi.Experiment.RegabiG => (LoweredGetG mem) // only lower in old ABI. in new ABI we have a G register.
+(GetG mem) && !(objabi.Experiment.RegabiG && v.Block.Func.OwnAux.Fn.ABI() == obj.ABIInternal) => (LoweredGetG mem) // only lower in old ABI. in new ABI we have a G register.
(GetClosurePtr ...) => (LoweredGetClosurePtr ...)
(GetCallerPC ...) => (LoweredGetCallerPC ...)
(GetCallerSP ...) => (LoweredGetCallerSP ...)
diff --git a/src/cmd/compile/internal/ssa/rewriteAMD64.go b/src/cmd/compile/internal/ssa/rewriteAMD64.go
index ce94fdb..1f56b70 100644
--- a/src/cmd/compile/internal/ssa/rewriteAMD64.go
+++ b/src/cmd/compile/internal/ssa/rewriteAMD64.go
@@ -4,6 +4,7 @@
package ssa
import "math"
+import "cmd/internal/obj"
import "cmd/internal/objabi"
import "cmd/compile/internal/types"
@@ -30465,11 +30466,11 @@
func rewriteValueAMD64_OpGetG(v *Value) bool {
v_0 := v.Args[0]
// match: (GetG mem)
- // cond: !objabi.Experiment.RegabiG
+ // cond: !(objabi.Experiment.RegabiG && v.Block.Func.OwnAux.Fn.ABI() == obj.ABIInternal)
// result: (LoweredGetG mem)
for {
mem := v_0
- if !(!objabi.Experiment.RegabiG) {
+ if !(!(objabi.Experiment.RegabiG && v.Block.Func.OwnAux.Fn.ABI() == obj.ABIInternal)) {
break
}
v.reset(OpAMD64LoweredGetG)
diff --git a/src/cmd/compile/internal/ssagen/abi.go b/src/cmd/compile/internal/ssagen/abi.go
index 9c20383..61d065c 100644
--- a/src/cmd/compile/internal/ssagen/abi.go
+++ b/src/cmd/compile/internal/ssagen/abi.go
@@ -146,6 +146,13 @@
fn.ABI = defABI
}
+ if fn.Pragma&ir.CgoUnsafeArgs != 0 {
+ // CgoUnsafeArgs indicates the function (or its callee) uses
+ // offsets to dispatch arguments, which currently using ABI0
+ // frame layout. Pin it to ABI0.
+ fn.ABI = obj.ABI0
+ }
+
// Apply references.
if abis, ok := s.refs[symName]; ok {
fn.ABIRefs |= abis
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
1 comment:
Patchset:
Thanks.
To view, visit change 306609. To unsubscribe, or for help writing mail filters, visit settings.