]> Cypherpunks repositories - gostls13.git/commitdiff
Revert "cmd/compile: improve stp merging for non-sequent cases"
authorKeith Randall <khr@golang.org>
Wed, 10 Sep 2025 17:39:17 +0000 (10:39 -0700)
committerGopher Robot <gobot@golang.org>
Wed, 10 Sep 2025 18:11:11 +0000 (11:11 -0700)
This reverts commit 4c63d798cb947a3cdd5a5b68f254a73d83eb288f.

Reason for revert: Causes miscompilations. See issue 75365.

Change-Id: Icd1fcfeb23d2ec524b16eb556030f43875e1c90d
Reviewed-on: https://go-review.googlesource.com/c/go/+/702455
Reviewed-by: Keith Randall <khr@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Mark Freeman <markfreeman@google.com>
src/cmd/compile/internal/ssa/pair.go
test/codegen/memcombine.go

index 0aa70e416f8f324d9c54d9f91c1c3fd06d52c514..83d7e476dd9c20712568715f1b8297a040881602 100644 (file)
@@ -212,12 +212,6 @@ func pairStores(f *Func) {
        last := f.Cache.allocBoolSlice(f.NumValues())
        defer f.Cache.freeBoolSlice(last)
 
-       type stChainElem struct {
-               v *Value
-               i int // Index in chain (0 == last store)
-       }
-       var order []stChainElem
-
        // prevStore returns the previous store in the
        // same block, or nil if there are none.
        prevStore := func(v *Value) *Value {
@@ -231,27 +225,6 @@ func pairStores(f *Func) {
                return m
        }
 
-       // storeWidth returns the width of store,
-       // or 0 if it is not a store
-       storeWidth := func(op Op) int64 {
-               var width int64
-               switch op {
-               case OpARM64MOVDstore, OpARM64FMOVDstore:
-                       width = 8
-               case OpARM64MOVWstore, OpARM64FMOVSstore:
-                       width = 4
-               case OpARM64MOVHstore:
-                       width = 2
-               case OpARM64MOVBstore:
-                       width = 1
-               default:
-                       width = 0
-               }
-               return width
-       }
-
-       const limit = 10
-
        for _, b := range f.Blocks {
                // Find last store in block, so we can
                // walk the stores last to first.
@@ -277,84 +250,9 @@ func pairStores(f *Func) {
                        }
                }
 
-               order = order[:0]
-               for i, v := 0, lastMem; v != nil; v = prevStore(v) {
-                       order = append(order, stChainElem{v, i})
-                       i++
-               }
-       reordering:
-               for i, v_elem := range order {
-                       v := v_elem.v
-                       if v.Uses != 1 {
-                               // We can't reorder stores if the earlier
-                               // store has any use besides the next one
-                               // in the store chain.
-                               // (Unless we could check the aliasing of
-                               // all those other uses.)
-                               continue
-                       }
-                       widthV := storeWidth(v.Op)
-                       if widthV == 0 {
-                               // Can't reorder with any other memory operations.
-                               // (atomics, calls, ...)
-                               continue
-                       }
-                       chain := order[i+1:]
-                       count := limit
-                       // Var 'count' keeps us in O(n) territory
-                       for j, w_elem := range chain {
-                               if count--; count == 0 {
-                                       // Only look back so far.
-                                       // This keeps us in O(n) territory, and it
-                                       // also prevents us from keeping values
-                                       // in registers for too long (and thus
-                                       // needing to spill them).
-                                       continue reordering
-                               }
-
-                               w := w_elem.v
-                               if w.Uses != 1 {
-                                       // We can't reorder stores if the earlier
-                                       // store has any use besides the next one
-                                       // in the store chain.
-                                       // (Unless we could check the aliasing of
-                                       // all those other uses.)
-                                       continue reordering
-                               }
-
-                               widthW := storeWidth(w.Op)
-                               if widthW == 0 {
-                                       // Can't reorder with any other memory operations.
-                                       // (atomics, calls, ...)
-                                       continue reordering
-                               }
-
-                               // We only allow reordering with respect to other
-                               // writes to the same pointer and aux, so we can
-                               // compute the exact the aliasing relationship.
-                               if w.Args[0] != v.Args[0] ||
-                                       w.Aux != v.Aux {
-                                       // Can't reorder with operation with incomparable destination memory pointer.
-                                       continue reordering
-                               }
-                               if overlap(w.AuxInt, widthW, v.AuxInt, widthV) {
-                                       // Aliases with the same slot with v's location.
-                                       continue reordering
-                               }
-
-                               // Reordering stores in increasing order of memory access
-                               if v.AuxInt < w.AuxInt {
-                                       order[i], order[i+j+1] = order[i+j+1], order[i]
-                                       v = w
-                                       widthV = widthW
-                               }
-                       }
-               }
-
                // Check all stores, from last to first.
        memCheck:
-               for i, v_elem := range order {
-                       v := v_elem.v
+               for v := lastMem; v != nil; v = prevStore(v) {
                        info := pairableStores[v.Op]
                        if info.width == 0 {
                                continue // Not pairable.
@@ -371,10 +269,8 @@ func pairStores(f *Func) {
                        // Look for earlier store we can combine with.
                        lowerOk := true
                        higherOk := true
-                       count := limit // max lookback distance
-                       chain := order[i+1:]
-                       for _, w_elem := range chain {
-                               w := w_elem.v
+                       count := 10 // max lookback distance
+                       for w := prevStore(v); w != nil; w = prevStore(w) {
                                if w.Uses != 1 {
                                        // We can't combine stores if the earlier
                                        // store has any use besides the next one
@@ -397,17 +293,11 @@ func pairStores(f *Func) {
                                                args[1], args[2] = args[2], args[1]
                                                off -= info.width
                                        }
-
                                        v.reset(info.pair)
                                        v.AddArgs(args...)
                                        v.Aux = aux
                                        v.AuxInt = off
-                                       // Take position of earlier of the two stores
-                                       if v_elem.i < w_elem.i {
-                                               v.Pos = w.Pos
-                                       } else {
-                                               w.Pos = v.Pos
-                                       }
+                                       v.Pos = w.Pos // take position of earlier of the two stores (TODO: not really working?)
 
                                        // Make w just a memory copy.
                                        wmem := w.MemoryArg()
index 9457d15f6a431ba0a43d9841d19eda3981a8b0b5..fa0e902ac2b485834f08a226c4a0ba528428d6c7 100644 (file)
@@ -1053,34 +1053,17 @@ func dwstoreF32(p *struct{ a, b float32 }, x, y float32) {
 }
 
 func dwstoreBig(p *struct{ a, b, c, d, e, f int64 }, a, b, c, d, e, f int64) {
-       // arm64:`STP\s\(R[0-9]+, R[0-9]+\), 16\(R[0-9]+\)`
+       // This is not perfect. We merge b+a, then d+e, then c and f have no pair.
        p.c = c
-       // arm64:`STP\s\(R[0-9]+, R[0-9]+\), 32\(R[0-9]+\)`
        p.f = f
        // arm64:`STP\s\(R[0-9]+, R[0-9]+\), \(R[0-9]+\)`
        p.a = a
+       // arm64:`STP\s\(R[0-9]+, R[0-9]+\), 24\(R[0-9]+\)`
        p.e = e
        p.d = d
        p.b = b
 }
 
-func dwstoreUnorderedArray(p *struct{ a, b, c, d int }, a, b, c, d int) {
-       // arm64:`STP\s\(R[0-9]+, R[0-9]+\), 16\(R[0-9]+\)`
-       p.c = c
-       p.d = d
-       // arm64:`STP\s\(R[0-9]+, R[0-9]+\), \(R[0-9]+\)`
-       p.a = a
-       p.b = b
-}
-
-func dwstoreBigNil(p *struct{ i, j struct{ a, b, c int } }) {
-       // arm64:`STP\s\(ZR, ZR\), 32\(R[0-9]+\)`
-       // arm64:`STP\s\(ZR, ZR\), 16\(R[0-9]+\)`
-       p.j = struct{ a, b, c int }{}
-       // arm64:`STP\s\(ZR, ZR\), \(R[0-9]+\)`
-       p.i = struct{ a, b, c int }{}
-}
-
 func dwstoreRet() [2]int {
        // arm64:"STP\t"
        return [2]int{5, 6}