[go] cmd/compile: fix AMD64 misscompilation due to AMD64 SETcc and Jcc mapping

2 views
Skip to first unread message

Jorropo (Gerrit)

unread,
Jun 4, 2026, 3:33:43 AM (3 days ago) Jun 4
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Jorropo has uploaded the change for review

Commit message

cmd/compile: fix AMD64 misscompilation due to AMD64 SETcc and Jcc mapping

Based on the following lines of code (483-484):
  // Special case for floating point - LF/LEF not generated
  (If (SETGF  cmp) yes no) => (UGT  cmp yes no)
  (If (SETGEF cmp) yes no) => (UGE  cmp yes no)

I had got it backward.

Theses conditional codes tables mappings are extremely error prone
and I already have an issue about that #56722 !

I really should dust off CL 450059, remove the cute cc handling
that were refused and just merge the harmonization.
Change-Id: Ib0cdedbd1bb0fe262e412134a979f57302ea8027

Change diff

diff --git a/src/cmd/compile/internal/ssa/_gen/AMD64.rules b/src/cmd/compile/internal/ssa/_gen/AMD64.rules
index dff5c93..8155951 100644
--- a/src/cmd/compile/internal/ssa/_gen/AMD64.rules
+++ b/src/cmd/compile/internal/ssa/_gen/AMD64.rules
@@ -1873,10 +1873,10 @@
 
 // remove flags → bool → flags roundtrip
 // Only do it if the flag generating instruction is local otherwise the likelihood flagalloc won't undo this optimization and makes things worse are slim.
-(NE t:(TESTQ x:(MOVBQZX s:(SET(EQ|NE|L|G|LE|GE|A|B|AE|BE|EQF|NEF|GF|GEF) flags)) x) yes no) && t.Block == s.Block => ((EQ|NE|LT|GT|LE|GE|UGT|ULT|UGE|ULE|EQF|NEF|UGE|UGT) flags yes no)
-(NE t:(TESTL x:(MOVBQZX s:(SET(EQ|NE|L|G|LE|GE|A|B|AE|BE|EQF|NEF|GF|GEF) flags)) x) yes no) && t.Block == s.Block => ((EQ|NE|LT|GT|LE|GE|UGT|ULT|UGE|ULE|EQF|NEF|UGE|UGT) flags yes no)
-(NE t:(TESTW x:(MOVBQZX s:(SET(EQ|NE|L|G|LE|GE|A|B|AE|BE|EQF|NEF|GF|GEF) flags)) x) yes no) && t.Block == s.Block => ((EQ|NE|LT|GT|LE|GE|UGT|ULT|UGE|ULE|EQF|NEF|UGE|UGT) flags yes no)
-(NE t:(TESTB            s:(SET(EQ|NE|L|G|LE|GE|A|B|AE|BE|EQF|NEF|GF|GEF) flags)  s) yes no) && t.Block == s.Block => ((EQ|NE|LT|GT|LE|GE|UGT|ULT|UGE|ULE|EQF|NEF|UGE|UGT) flags yes no)
+(NE t:(TESTQ x:(MOVBQZX s:(SET(EQ|NE|L|G|LE|GE|A|B|AE|BE|EQF|NEF|GF|GEF) flags)) x) yes no) && t.Block == s.Block => ((EQ|NE|LT|GT|LE|GE|UGT|ULT|UGE|ULE|EQF|NEF|UGT|UGE) flags yes no)
+(NE t:(TESTL x:(MOVBQZX s:(SET(EQ|NE|L|G|LE|GE|A|B|AE|BE|EQF|NEF|GF|GEF) flags)) x) yes no) && t.Block == s.Block => ((EQ|NE|LT|GT|LE|GE|UGT|ULT|UGE|ULE|EQF|NEF|UGT|UGE) flags yes no)
+(NE t:(TESTW x:(MOVBQZX s:(SET(EQ|NE|L|G|LE|GE|A|B|AE|BE|EQF|NEF|GF|GEF) flags)) x) yes no) && t.Block == s.Block => ((EQ|NE|LT|GT|LE|GE|UGT|ULT|UGE|ULE|EQF|NEF|UGT|UGE) flags yes no)
+(NE t:(TESTB            s:(SET(EQ|NE|L|G|LE|GE|A|B|AE|BE|EQF|NEF|GF|GEF) flags)  s) yes no) && t.Block == s.Block => ((EQ|NE|LT|GT|LE|GE|UGT|ULT|UGE|ULE|EQF|NEF|UGT|UGE) flags yes no)
 
 (CMOVQNE yes no t:(TESTQ x:(MOVBQZX s:(SET(EQ|NE|L|G|LE|GE|A|B|AE|BE|EQF|NEF|GF|GEF) flags)) x)) && t.Block == s.Block => (CMOVQ(EQ|NE|LT|GT|LE|GE|HI|CS|CC|LS|EQF|NEF|GTF|GEF) yes no flags)
 (CMOVQNE yes no t:(TESTL x:(MOVBQZX s:(SET(EQ|NE|L|G|LE|GE|A|B|AE|BE|EQF|NEF|GF|GEF) flags)) x)) && t.Block == s.Block => (CMOVQ(EQ|NE|LT|GT|LE|GE|HI|CS|CC|LS|EQF|NEF|GTF|GEF) yes no flags)
diff --git a/src/cmd/compile/internal/ssa/rewriteAMD64.go b/src/cmd/compile/internal/ssa/rewriteAMD64.go
index 9a45de3..147fd98 100644
--- a/src/cmd/compile/internal/ssa/rewriteAMD64.go
+++ b/src/cmd/compile/internal/ssa/rewriteAMD64.go
@@ -108729,7 +108729,7 @@
 		}
 		// match: (NE t:(TESTQ x:(MOVBQZX s:(SETGF flags)) x) yes no)
 		// cond: t.Block == s.Block
-		// result: (UGE flags yes no)
+		// result: (UGT flags yes no)
 		for b.Controls[0].Op == OpAMD64TESTQ {
 			t := b.Controls[0]
 			_ = t.Args[1]
@@ -108748,14 +108748,14 @@
 				if x != t_1 || !(t.Block == s.Block) {
 					continue
 				}
-				b.resetWithControl(BlockAMD64UGE, flags)
+				b.resetWithControl(BlockAMD64UGT, flags)
 				return true
 			}
 			break
 		}
 		// match: (NE t:(TESTQ x:(MOVBQZX s:(SETGEF flags)) x) yes no)
 		// cond: t.Block == s.Block
-		// result: (UGT flags yes no)
+		// result: (UGE flags yes no)
 		for b.Controls[0].Op == OpAMD64TESTQ {
 			t := b.Controls[0]
 			_ = t.Args[1]
@@ -108774,7 +108774,7 @@
 				if x != t_1 || !(t.Block == s.Block) {
 					continue
 				}
-				b.resetWithControl(BlockAMD64UGT, flags)
+				b.resetWithControl(BlockAMD64UGE, flags)
 				return true
 			}
 			break
@@ -109093,7 +109093,7 @@
 		}
 		// match: (NE t:(TESTL x:(MOVBQZX s:(SETGF flags)) x) yes no)
 		// cond: t.Block == s.Block
-		// result: (UGE flags yes no)
+		// result: (UGT flags yes no)
 		for b.Controls[0].Op == OpAMD64TESTL {
 			t := b.Controls[0]
 			_ = t.Args[1]
@@ -109112,14 +109112,14 @@
 				if x != t_1 || !(t.Block == s.Block) {
 					continue
 				}
-				b.resetWithControl(BlockAMD64UGE, flags)
+				b.resetWithControl(BlockAMD64UGT, flags)
 				return true
 			}
 			break
 		}
 		// match: (NE t:(TESTL x:(MOVBQZX s:(SETGEF flags)) x) yes no)
 		// cond: t.Block == s.Block
-		// result: (UGT flags yes no)
+		// result: (UGE flags yes no)
 		for b.Controls[0].Op == OpAMD64TESTL {
 			t := b.Controls[0]
 			_ = t.Args[1]
@@ -109138,7 +109138,7 @@
 				if x != t_1 || !(t.Block == s.Block) {
 					continue
 				}
-				b.resetWithControl(BlockAMD64UGT, flags)
+				b.resetWithControl(BlockAMD64UGE, flags)
 				return true
 			}
 			break
@@ -109457,7 +109457,7 @@
 		}
 		// match: (NE t:(TESTW x:(MOVBQZX s:(SETGF flags)) x) yes no)
 		// cond: t.Block == s.Block
-		// result: (UGE flags yes no)
+		// result: (UGT flags yes no)
 		for b.Controls[0].Op == OpAMD64TESTW {
 			t := b.Controls[0]
 			_ = t.Args[1]
@@ -109476,14 +109476,14 @@
 				if x != t_1 || !(t.Block == s.Block) {
 					continue
 				}
-				b.resetWithControl(BlockAMD64UGE, flags)
+				b.resetWithControl(BlockAMD64UGT, flags)
 				return true
 			}
 			break
 		}
 		// match: (NE t:(TESTW x:(MOVBQZX s:(SETGEF flags)) x) yes no)
 		// cond: t.Block == s.Block
-		// result: (UGT flags yes no)
+		// result: (UGE flags yes no)
 		for b.Controls[0].Op == OpAMD64TESTW {
 			t := b.Controls[0]
 			_ = t.Args[1]
@@ -109502,7 +109502,7 @@
 				if x != t_1 || !(t.Block == s.Block) {
 					continue
 				}
-				b.resetWithControl(BlockAMD64UGT, flags)
+				b.resetWithControl(BlockAMD64UGE, flags)
 				return true
 			}
 			break
@@ -109773,7 +109773,7 @@
 		}
 		// match: (NE t:(TESTB s:(SETGF flags) s) yes no)
 		// cond: t.Block == s.Block
-		// result: (UGE flags yes no)
+		// result: (UGT flags yes no)
 		for b.Controls[0].Op == OpAMD64TESTB {
 			t := b.Controls[0]
 			_ = t.Args[1]
@@ -109788,14 +109788,14 @@
 				if s != t_1 || !(t.Block == s.Block) {
 					continue
 				}
-				b.resetWithControl(BlockAMD64UGE, flags)
+				b.resetWithControl(BlockAMD64UGT, flags)
 				return true
 			}
 			break
 		}
 		// match: (NE t:(TESTB s:(SETGEF flags) s) yes no)
 		// cond: t.Block == s.Block
-		// result: (UGT flags yes no)
+		// result: (UGE flags yes no)
 		for b.Controls[0].Op == OpAMD64TESTB {
 			t := b.Controls[0]
 			_ = t.Args[1]
@@ -109810,7 +109810,7 @@
 				if s != t_1 || !(t.Block == s.Block) {
 					continue
 				}
-				b.resetWithControl(BlockAMD64UGT, flags)
+				b.resetWithControl(BlockAMD64UGE, flags)
 				return true
 			}
 			break

Change information

Files:
  • M src/cmd/compile/internal/ssa/_gen/AMD64.rules
  • M src/cmd/compile/internal/ssa/rewriteAMD64.go
Change size: S
Delta: 2 files changed, 20 insertions(+), 20 deletions(-)
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newchange
Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Change-Id: Ib0cdedbd1bb0fe262e412134a979f57302ea8027
Gerrit-Change-Number: 786860
Gerrit-PatchSet: 1
Gerrit-Owner: Jorropo <jorro...@gmail.com>
Gerrit-Reviewer: Jorropo <jorro...@gmail.com>
unsatisfied_requirement
satisfied_requirement
open
diffy

Jorropo (Gerrit)

unread,
Jun 4, 2026, 3:45:15 AM (3 days ago) Jun 4
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com

Jorropo added 1 comment

Commit Message
Line 20, Patchset 1 (Latest):that were refused and just merge the harmonization.
Jorropo . resolved

Turns out I never wrote the hamonization only the `cc` code.

Well the sentence is correct I just need to do the harmonization from scratch.

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Change-Id: Ib0cdedbd1bb0fe262e412134a979f57302ea8027
Gerrit-Change-Number: 786860
Gerrit-PatchSet: 1
Gerrit-Owner: Jorropo <jorro...@gmail.com>
Gerrit-Reviewer: Jorropo <jorro...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jun 2026 07:45:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
unsatisfied_requirement
satisfied_requirement
open
diffy

Jorropo (Gerrit)

unread,
Jun 5, 2026, 11:58:40 PM (2 days ago) Jun 5
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Jorropo, Keith Randall and Martin Möhrmann

Jorropo uploaded new patchset

Jorropo uploaded patch set #2 to this change.
Following approvals got outdated and were removed:
Open in Gerrit

Related details

Attention is currently required from:
  • Jorropo
  • Keith Randall
  • Martin Möhrmann
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newpatchset
Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Change-Id: Ib0cdedbd1bb0fe262e412134a979f57302ea8027
Gerrit-Change-Number: 786860
Gerrit-PatchSet: 2
Gerrit-Owner: Jorropo <jorro...@gmail.com>
Gerrit-Reviewer: Jorropo <jorro...@gmail.com>
Gerrit-Reviewer: Keith Randall <k...@golang.org>
Gerrit-Reviewer: Martin Möhrmann <moeh...@google.com>
Gerrit-CC: Gopher Robot <go...@golang.org>
Gerrit-Attention: Keith Randall <k...@golang.org>
Gerrit-Attention: Jorropo <jorro...@gmail.com>
Gerrit-Attention: Martin Möhrmann <moeh...@google.com>
unsatisfied_requirement
satisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages