Skip to content

Commit dbfa3ca

Browse files
randall77randall77
committed
cmd/compile: fix typing of atomic logical operations
For atomic AND and OR operations on memory, we currently have two views of the op. One just does the operation on the memory and returns just a memory. The other does the operation on the memory and returns the old value (before having the logical operation done to it) and memory. Update #61395 These two type differently, and there's currently some confusion in our rules about which is which. Use different names for the two different flavors so we don't get them confused. Change-Id: I07b4542db672b2cee98169ac42b67db73c482093 Reviewed-on: https://go-review.googlesource.com/c/go/+/594976 Reviewed-by: Cherry Mui <[email protected]> Reviewed-by: Nicolas Hillegeer <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Mauri de Souza Meneguzzo <[email protected]> Reviewed-by: Keith Randall <[email protected]>
1 parent df009ee commit dbfa3ca

File tree

5 files changed

+113
-67
lines changed

5 files changed

+113
-67
lines changed

‎src/cmd/compile/internal/ssa/_gen/ARM64.rules

+4-4
Original file line numberDiff line numberDiff line change
@@ -580,10 +580,10 @@
580580
(AtomicCompareAndSwap(32|64)Variant ...) => (LoweredAtomicCas(32|64)Variant ...)
581581

582582
// Return old contents.
583-
(AtomicAnd(64|32|8) ...) => (LoweredAtomicAnd(64|32|8) ...)
584-
(AtomicOr(64|32|8) ...) => (LoweredAtomicOr(64|32|8) ...)
585-
(AtomicAnd(64|32|8)Variant ...) => (LoweredAtomicAnd(64|32|8)Variant ...)
586-
(AtomicOr(64|32|8)Variant ...) => (LoweredAtomicOr(64|32|8)Variant ...)
583+
(AtomicAnd(64|32|8)value ...) => (LoweredAtomicAnd(64|32|8) ...)
584+
(AtomicOr(64|32|8)value ...) => (LoweredAtomicOr(64|32|8) ...)
585+
(AtomicAnd(64|32|8)valueVariant ...) => (LoweredAtomicAnd(64|32|8)Variant ...)
586+
(AtomicOr(64|32|8)valueVariant ...) => (LoweredAtomicOr(64|32|8)Variant ...)
587587

588588
// Write barrier.
589589
(WB ...) => (LoweredWB ...)

‎src/cmd/compile/internal/ssa/_gen/genericOps.go

+20-12
Original file line numberDiff line numberDiff line change
@@ -609,12 +609,20 @@ var genericOps = []opData{
609609
{name: "AtomicCompareAndSwap32", argLength: 4, typ: "(Bool,Mem)", hasSideEffects: true}, // if *arg0==arg1, then set *arg0=arg2. Returns true if store happens and new memory.
610610
{name: "AtomicCompareAndSwap64", argLength: 4, typ: "(Bool,Mem)", hasSideEffects: true}, // if *arg0==arg1, then set *arg0=arg2. Returns true if store happens and new memory.
611611
{name: "AtomicCompareAndSwapRel32", argLength: 4, typ: "(Bool,Mem)", hasSideEffects: true}, // if *arg0==arg1, then set *arg0=arg2. Lock release, reports whether store happens and new memory.
612-
{name: "AtomicAnd8", argLength: 3, typ: "(Uint8, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
613-
{name: "AtomicOr8", argLength: 3, typ: "(Uint8, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
614-
{name: "AtomicAnd64", argLength: 3, typ: "(Uint64, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
615-
{name: "AtomicAnd32", argLength: 3, typ: "(Uint32, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
616-
{name: "AtomicOr64", argLength: 3, typ: "(Uint64, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
617-
{name: "AtomicOr32", argLength: 3, typ: "(Uint32, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
612+
613+
// Older atomic logical operations which don't return the old value.
614+
{name: "AtomicAnd8", argLength: 3, typ: "Mem", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns memory.
615+
{name: "AtomicOr8", argLength: 3, typ: "Mem", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns memory.
616+
{name: "AtomicAnd32", argLength: 3, typ: "Mem", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns memory.
617+
{name: "AtomicOr32", argLength: 3, typ: "Mem", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns memory.
618+
619+
// Newer atomic logical operations which return the old value.
620+
{name: "AtomicAnd64value", argLength: 3, typ: "(Uint64, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
621+
{name: "AtomicAnd32value", argLength: 3, typ: "(Uint32, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
622+
{name: "AtomicAnd8value", argLength: 3, typ: "(Uint8, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
623+
{name: "AtomicOr64value", argLength: 3, typ: "(Uint64, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
624+
{name: "AtomicOr32value", argLength: 3, typ: "(Uint32, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
625+
{name: "AtomicOr8value", argLength: 3, typ: "(Uint8, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
618626

619627
// Atomic operation variants
620628
// These variants have the same semantics as above atomic operations.
@@ -628,12 +636,12 @@ var genericOps = []opData{
628636
{name: "AtomicExchange64Variant", argLength: 3, typ: "(UInt64,Mem)", hasSideEffects: true}, // Store arg1 to *arg0. arg2=memory. Returns old contents of *arg0 and new memory.
629637
{name: "AtomicCompareAndSwap32Variant", argLength: 4, typ: "(Bool,Mem)", hasSideEffects: true}, // if *arg0==arg1, then set *arg0=arg2. Returns true if store happens and new memory.
630638
{name: "AtomicCompareAndSwap64Variant", argLength: 4, typ: "(Bool,Mem)", hasSideEffects: true}, // if *arg0==arg1, then set *arg0=arg2. Returns true if store happens and new memory.
631-
{name: "AtomicAnd8Variant", argLength: 3, typ: "(Uint8, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
632-
{name: "AtomicOr8Variant", argLength: 3, typ: "(Uint8, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
633-
{name: "AtomicAnd64Variant", argLength: 3, typ: "(Uint64, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
634-
{name: "AtomicOr64Variant", argLength: 3, typ: "(Uint64, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
635-
{name: "AtomicAnd32Variant", argLength: 3, typ: "(Uint32, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
636-
{name: "AtomicOr32Variant", argLength: 3, typ: "(Uint32, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
639+
{name: "AtomicAnd64valueVariant", argLength: 3, typ: "(Uint64, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
640+
{name: "AtomicOr64valueVariant", argLength: 3, typ: "(Uint64, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
641+
{name: "AtomicAnd32valueVariant", argLength: 3, typ: "(Uint32, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
642+
{name: "AtomicOr32valueVariant", argLength: 3, typ: "(Uint32, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
643+
{name: "AtomicAnd8valueVariant", argLength: 3, typ: "(Uint8, Mem)", hasSideEffects: true}, // *arg0 &= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
644+
{name: "AtomicOr8valueVariant", argLength: 3, typ: "(Uint8, Mem)", hasSideEffects: true}, // *arg0 |= arg1. arg2=memory. Returns old contents of *arg0 and new memory.
637645

638646
// Publication barrier
639647
{name: "PubBarrier", argLength: 1, hasSideEffects: true}, // Do data barrier. arg0=memory.

‎src/cmd/compile/internal/ssa/opGen.go

+46-18
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/cmd/compile/internal/ssa/rewriteARM64.go

+12-12
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/cmd/compile/internal/ssagen/ssa.go

+31-21
Original file line numberDiff line numberDiff line change
@@ -4410,13 +4410,13 @@ func InitTables() {
44104410
},
44114411
sys.AMD64, sys.Loong64, sys.MIPS64, sys.PPC64, sys.RISCV64, sys.S390X)
44124412

4413-
type atomicOpEmitter func(s *state, n *ir.CallExpr, args []*ssa.Value, op ssa.Op, typ types.Kind)
4413+
type atomicOpEmitter func(s *state, n *ir.CallExpr, args []*ssa.Value, op ssa.Op, typ types.Kind, needReturn bool)
44144414

4415-
makeAtomicGuardedIntrinsicARM64 := func(op0, op1 ssa.Op, typ types.Kind, emit atomicOpEmitter) intrinsicBuilder {
4415+
makeAtomicGuardedIntrinsicARM64common := func(op0, op1 ssa.Op, typ types.Kind, emit atomicOpEmitter, needReturn bool) intrinsicBuilder {
44164416

44174417
return func(s *state, n *ir.CallExpr, args []*ssa.Value) *ssa.Value {
44184418
if buildcfg.GOARM64.LSE {
4419-
emit(s, n, args, op1, typ)
4419+
emit(s, n, args, op1, typ, needReturn)
44204420
} else {
44214421
// Target Atomic feature is identified by dynamic detection
44224422
addr := s.entryNewValue1A(ssa.OpAddr, types.Types[types.TBOOL].PtrTo(), ir.Syms.ARM64HasATOMICS, s.sb)
@@ -4433,29 +4433,37 @@ func InitTables() {
44334433

44344434
// We have atomic instructions - use it directly.
44354435
s.startBlock(bTrue)
4436-
emit(s, n, args, op1, typ)
4436+
emit(s, n, args, op1, typ, needReturn)
44374437
s.endBlock().AddEdgeTo(bEnd)
44384438

44394439
// Use original instruction sequence.
44404440
s.startBlock(bFalse)
4441-
emit(s, n, args, op0, typ)
4441+
emit(s, n, args, op0, typ, needReturn)
44424442
s.endBlock().AddEdgeTo(bEnd)
44434443

44444444
// Merge results.
44454445
s.startBlock(bEnd)
44464446
}
4447-
if typ == types.TNIL {
4448-
return nil
4449-
} else {
4447+
if needReturn {
44504448
return s.variable(n, types.Types[typ])
4449+
} else {
4450+
return nil
44514451
}
44524452
}
44534453
}
4454+
makeAtomicGuardedIntrinsicARM64 := func(op0, op1 ssa.Op, typ types.Kind, emit atomicOpEmitter) intrinsicBuilder {
4455+
return makeAtomicGuardedIntrinsicARM64common(op0, op1, typ, emit, true)
4456+
}
4457+
makeAtomicGuardedIntrinsicARM64old := func(op0, op1 ssa.Op, typ types.Kind, emit atomicOpEmitter) intrinsicBuilder {
4458+
return makeAtomicGuardedIntrinsicARM64common(op0, op1, typ, emit, false)
4459+
}
44544460

4455-
atomicEmitterARM64 := func(s *state, n *ir.CallExpr, args []*ssa.Value, op ssa.Op, typ types.Kind) {
4461+
atomicEmitterARM64 := func(s *state, n *ir.CallExpr, args []*ssa.Value, op ssa.Op, typ types.Kind, needReturn bool) {
44564462
v := s.newValue3(op, types.NewTuple(types.Types[typ], types.TypeMem), args[0], args[1], s.mem())
44574463
s.vars[memVar] = s.newValue1(ssa.OpSelect1, types.TypeMem, v)
4458-
s.vars[n] = s.newValue1(ssa.OpSelect0, types.Types[typ], v)
4464+
if needReturn {
4465+
s.vars[n] = s.newValue1(ssa.OpSelect0, types.Types[typ], v)
4466+
}
44594467
}
44604468
addF("internal/runtime/atomic", "Xchg",
44614469
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicExchange32, ssa.OpAtomicExchange32Variant, types.TUINT32, atomicEmitterARM64),
@@ -4508,10 +4516,12 @@ func InitTables() {
45084516
},
45094517
sys.PPC64)
45104518

4511-
atomicCasEmitterARM64 := func(s *state, n *ir.CallExpr, args []*ssa.Value, op ssa.Op, typ types.Kind) {
4519+
atomicCasEmitterARM64 := func(s *state, n *ir.CallExpr, args []*ssa.Value, op ssa.Op, typ types.Kind, needReturn bool) {
45124520
v := s.newValue4(op, types.NewTuple(types.Types[types.TBOOL], types.TypeMem), args[0], args[1], args[2], s.mem())
45134521
s.vars[memVar] = s.newValue1(ssa.OpSelect1, types.TypeMem, v)
4514-
s.vars[n] = s.newValue1(ssa.OpSelect0, types.Types[typ], v)
4522+
if needReturn {
4523+
s.vars[n] = s.newValue1(ssa.OpSelect0, types.Types[typ], v)
4524+
}
45154525
}
45164526

45174527
addF("internal/runtime/atomic", "Cas",
@@ -4538,7 +4548,7 @@ func InitTables() {
45384548
s.vars[memVar] = s.newValue3(ssa.OpAtomicOr8, types.TypeMem, args[0], args[1], s.mem())
45394549
return nil
45404550
},
4541-
sys.AMD64, sys.ARM64, sys.MIPS, sys.MIPS64, sys.PPC64, sys.RISCV64, sys.S390X)
4551+
sys.AMD64, sys.MIPS, sys.MIPS64, sys.PPC64, sys.RISCV64, sys.S390X)
45424552
addF("internal/runtime/atomic", "Or",
45434553
func(s *state, n *ir.CallExpr, args []*ssa.Value) *ssa.Value {
45444554
s.vars[memVar] = s.newValue3(ssa.OpAtomicOr32, types.TypeMem, args[0], args[1], s.mem())
@@ -4547,28 +4557,28 @@ func InitTables() {
45474557
sys.AMD64, sys.MIPS, sys.MIPS64, sys.PPC64, sys.RISCV64, sys.S390X)
45484558

45494559
addF("internal/runtime/atomic", "And8",
4550-
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicAnd8, ssa.OpAtomicAnd8Variant, types.TUINT8, atomicEmitterARM64),
4560+
makeAtomicGuardedIntrinsicARM64old(ssa.OpAtomicAnd8value, ssa.OpAtomicAnd8valueVariant, types.TUINT8, atomicEmitterARM64),
45514561
sys.ARM64)
45524562
addF("internal/runtime/atomic", "Or8",
4553-
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicOr8, ssa.OpAtomicOr8Variant, types.TUINT8, atomicEmitterARM64),
4563+
makeAtomicGuardedIntrinsicARM64old(ssa.OpAtomicOr8value, ssa.OpAtomicOr8valueVariant, types.TUINT8, atomicEmitterARM64),
45544564
sys.ARM64)
45554565
addF("internal/runtime/atomic", "And64",
4556-
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicAnd64, ssa.OpAtomicAnd64Variant, types.TUINT64, atomicEmitterARM64),
4566+
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicAnd64value, ssa.OpAtomicAnd64valueVariant, types.TUINT64, atomicEmitterARM64),
45574567
sys.ARM64)
45584568
addF("internal/runtime/atomic", "And32",
4559-
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicAnd32, ssa.OpAtomicAnd32Variant, types.TUINT32, atomicEmitterARM64),
4569+
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicAnd32value, ssa.OpAtomicAnd32valueVariant, types.TUINT32, atomicEmitterARM64),
45604570
sys.ARM64)
45614571
addF("internal/runtime/atomic", "And",
4562-
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicAnd32, ssa.OpAtomicAnd32Variant, types.TUINT32, atomicEmitterARM64),
4572+
makeAtomicGuardedIntrinsicARM64old(ssa.OpAtomicAnd32value, ssa.OpAtomicAnd32valueVariant, types.TUINT32, atomicEmitterARM64),
45634573
sys.ARM64)
45644574
addF("internal/runtime/atomic", "Or64",
4565-
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicOr64, ssa.OpAtomicOr64Variant, types.TUINT64, atomicEmitterARM64),
4575+
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicOr64value, ssa.OpAtomicOr64valueVariant, types.TUINT64, atomicEmitterARM64),
45664576
sys.ARM64)
45674577
addF("internal/runtime/atomic", "Or32",
4568-
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicOr32, ssa.OpAtomicOr32Variant, types.TUINT32, atomicEmitterARM64),
4578+
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicOr32value, ssa.OpAtomicOr32valueVariant, types.TUINT32, atomicEmitterARM64),
45694579
sys.ARM64)
45704580
addF("internal/runtime/atomic", "Or",
4571-
makeAtomicGuardedIntrinsicARM64(ssa.OpAtomicOr32, ssa.OpAtomicOr32Variant, types.TUINT32, atomicEmitterARM64),
4581+
makeAtomicGuardedIntrinsicARM64old(ssa.OpAtomicOr32value, ssa.OpAtomicOr32valueVariant, types.TUINT32, atomicEmitterARM64),
45724582
sys.ARM64)
45734583

45744584
// Aliases for atomic load operations

0 commit comments

Comments
 (0)