Skip to content

Commit a3295df

Browse files
DanielMorsingrandall77
authored andcommitted
cmd/compile/internal/ssa: Use transitive properties for len/cap
Remove the special casing for len/cap and rely on the posets. After removing the special logic, I ran `go build -gcflags='-d ssa/prove/debug=2' all` to verify my results. During this, I found 2 common cases where the old implicit unsigned->signed domain conversion made proving a branch possible that shouldn't be strictly possible and added these. The 2 cases are shifting a non-negative signed integer and unsigned comparisons that happen with arguments that fits entirely inside the unsigned argument Change-Id: Ic88049ff69efc5602fc15f5dad02028e704f5483 Reviewed-on: https://go-review.googlesource.com/c/go/+/679155 Reviewed-by: Mark Freeman <[email protected]> Reviewed-by: Keith Randall <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Keith Randall <[email protected]>
1 parent bd08285 commit a3295df

File tree

1 file changed

+32
-41
lines changed

1 file changed

+32
-41
lines changed

src/cmd/compile/internal/ssa/prove.go

Lines changed: 32 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -409,12 +409,6 @@ type factsTable struct {
409409
limits []limit // indexed by value ID
410410
limitStack []limitFact // previous entries
411411
recurseCheck []bool // recursion detector for limit propagation
412-
413-
// For each slice s, a map from s to a len(s)/cap(s) value (if any)
414-
// TODO: check if there are cases that matter where we have
415-
// more than one len(s) for a slice. We could keep a list if necessary.
416-
lens map[ID]*Value
417-
caps map[ID]*Value
418412
}
419413

420414
// checkpointBound is an invalid value used for checkpointing
@@ -666,6 +660,11 @@ func (ft *factsTable) newLimit(v *Value, newLim limit) bool {
666660
}
667661
if !isTrue {
668662
r ^= lt | gt | eq
663+
} else if d == unsigned && (r == lt || r == lt|eq) && ft.isNonNegative(v.Args[1]) {
664+
// Since every representation of a non-negative signed number is the same
665+
// as in the unsigned domain, we can transfer x <= y to the signed domain,
666+
// but only for the true branch.
667+
d |= signed
669668
}
670669
// TODO: v.Block is wrong?
671670
addRestrictions(v.Block, ft, d, v.Args[0], v.Args[1], r)
@@ -730,7 +729,7 @@ func (ft *factsTable) addOrdering(v, w *Value, d domain, r relation) {
730729
// restricting it to r.
731730
func (ft *factsTable) update(parent *Block, v, w *Value, d domain, r relation) {
732731
if parent.Func.pass.debug > 2 {
733-
parent.Func.Warnl(parent.Pos, "parent=%s, update %s %s %s", parent, v, w, r)
732+
parent.Func.Warnl(parent.Pos, "parent=%s, update %s %s %s %s", parent, d, v, w, r)
734733
}
735734
// No need to do anything else if we already found unsat.
736735
if ft.unsat {
@@ -942,32 +941,6 @@ func (ft *factsTable) update(parent *Block, v, w *Value, d domain, r relation) {
942941
return
943942
}
944943

945-
// Additional facts we know given the relationship between len and cap.
946-
//
947-
// TODO: Since prove now derives transitive relations, it
948-
// should be sufficient to learn that len(w) <= cap(w) at the
949-
// beginning of prove where we look for all len/cap ops.
950-
if v.Op == OpSliceLen && r&lt == 0 && ft.caps[v.Args[0].ID] != nil {
951-
// len(s) > w implies cap(s) > w
952-
// len(s) >= w implies cap(s) >= w
953-
// len(s) == w implies cap(s) >= w
954-
ft.update(parent, ft.caps[v.Args[0].ID], w, d, r|gt)
955-
}
956-
if w.Op == OpSliceLen && r&gt == 0 && ft.caps[w.Args[0].ID] != nil {
957-
// same, length on the RHS.
958-
ft.update(parent, v, ft.caps[w.Args[0].ID], d, r|lt)
959-
}
960-
if v.Op == OpSliceCap && r&gt == 0 && ft.lens[v.Args[0].ID] != nil {
961-
// cap(s) < w implies len(s) < w
962-
// cap(s) <= w implies len(s) <= w
963-
// cap(s) == w implies len(s) <= w
964-
ft.update(parent, ft.lens[v.Args[0].ID], w, d, r|lt)
965-
}
966-
if w.Op == OpSliceCap && r&lt == 0 && ft.lens[w.Args[0].ID] != nil {
967-
// same, capacity on the RHS.
968-
ft.update(parent, v, ft.lens[w.Args[0].ID], d, r|gt)
969-
}
970-
971944
// Process fence-post implications.
972945
//
973946
// First, make the condition > or >=.
@@ -1421,6 +1394,8 @@ func prove(f *Func) {
14211394
ft := newFactsTable(f)
14221395
ft.checkpoint()
14231396

1397+
var lens map[ID]*Value
1398+
var caps map[ID]*Value
14241399
// Find length and capacity ops.
14251400
for _, b := range f.Blocks {
14261401
for _, v := range b.Values {
@@ -1431,26 +1406,39 @@ func prove(f *Func) {
14311406
}
14321407
switch v.Op {
14331408
case OpSliceLen:
1434-
if ft.lens == nil {
1435-
ft.lens = map[ID]*Value{}
1409+
if lens == nil {
1410+
lens = map[ID]*Value{}
14361411
}
14371412
// Set all len Values for the same slice as equal in the poset.
14381413
// The poset handles transitive relations, so Values related to
14391414
// any OpSliceLen for this slice will be correctly related to others.
1440-
if l, ok := ft.lens[v.Args[0].ID]; ok {
1415+
//
1416+
// Since we know that lens/caps are non-negative, their relation
1417+
// can be added in both the signed and unsigned domain.
1418+
if l, ok := lens[v.Args[0].ID]; ok {
14411419
ft.update(b, v, l, signed, eq)
1420+
ft.update(b, v, l, unsigned, eq)
14421421
} else {
1443-
ft.lens[v.Args[0].ID] = v
1422+
lens[v.Args[0].ID] = v
1423+
}
1424+
if c, ok := caps[v.Args[0].ID]; ok {
1425+
ft.update(b, v, c, signed, lt|eq)
1426+
ft.update(b, v, c, unsigned, lt|eq)
14441427
}
14451428
case OpSliceCap:
1446-
if ft.caps == nil {
1447-
ft.caps = map[ID]*Value{}
1429+
if caps == nil {
1430+
caps = map[ID]*Value{}
14481431
}
14491432
// Same as case OpSliceLen above, but for slice cap.
1450-
if c, ok := ft.caps[v.Args[0].ID]; ok {
1433+
if c, ok := caps[v.Args[0].ID]; ok {
14511434
ft.update(b, v, c, signed, eq)
1435+
ft.update(b, v, c, unsigned, eq)
14521436
} else {
1453-
ft.caps[v.Args[0].ID] = v
1437+
caps[v.Args[0].ID] = v
1438+
}
1439+
if l, ok := lens[v.Args[0].ID]; ok {
1440+
ft.update(b, v, l, signed, gt|eq)
1441+
ft.update(b, v, l, unsigned, gt|eq)
14541442
}
14551443
}
14561444
}
@@ -2260,6 +2248,9 @@ func addLocalFacts(ft *factsTable, b *Block) {
22602248
OpRsh32Ux64, OpRsh32Ux32, OpRsh32Ux16, OpRsh32Ux8,
22612249
OpRsh64Ux64, OpRsh64Ux32, OpRsh64Ux16, OpRsh64Ux8:
22622250
ft.update(b, v, v.Args[0], unsigned, lt|eq)
2251+
if ft.isNonNegative(v.Args[0]) {
2252+
ft.update(b, v, v.Args[0], signed, lt|eq)
2253+
}
22632254
case OpMod64u, OpMod32u, OpMod16u, OpMod8u:
22642255
ft.update(b, v, v.Args[0], unsigned, lt|eq)
22652256
// Note: we have to be careful that this doesn't imply

0 commit comments

Comments
 (0)