Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: AMD64 and i386 conditional suffixes of Ops are inconsistent #56722

Open
Jorropo opened this issue Nov 13, 2022 · 2 comments
Open
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@Jorropo
Copy link
Contributor

Jorropo commented Nov 13, 2022

What version of Go are you using (go version)?

$ go version
go version devel go1.20-51834965b7 Sun Nov 13 02:26:29 2022 +0100 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/hugo/.cache/go-build"
GOENV="/home/hugo/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/hugo/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/hugo/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/home/hugo/k/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/hugo/k/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.20-51834965b7 Sun Nov 13 02:26:29 2022 +0100"
GCCGO="gccgo"
GOAMD64="v3"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/hugo/Documents/Scripts/go/src/cmd/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build125027850=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I was refactoring some SSA rules to canonicalize inversed comparision Ops similar to how LLVM do it.
(for example (NE cond yes no) => (EQ cond no yes))
This aims to simplify rules because we would have to handle only half as many rules for thoses that touch flags.

What did you expect to see?

I am expecting to see similar suffixes names between all instructions that use flag suffixes, similar to how the intel manual do Jcc, SETcc, CMOVcc, ...

What did you see instead?

Blocks (Jcc), CMOV and SET sometime use different names for the flags suffixes.

// Legend
Op Name -> Assembly Mnemonic
!MISSING! // this indicate that the arch have the instruction but we don't have an ssa Op for it
?MISSING? // this indicate that there is no corresponding ssa Op but there is probably a good reason behind it

// ZF == 0, good
SETEQ   -> SETEQ
CMOVQEQ -> CMOVQEQ
EQ      -> JEQ

// ZF == 1, good
SETNE   -> SETNE
CMOVQNE -> CMOVQNE
NE      -> JNE

// CF == 0 && ZF == 0
SETA    -> SETHI   // different
CMOVQHI -> CMOVQHI
UGT     -> JHI     // different

// CF == 1 || ZF == 1
SETBE   -> SETLS   // different
CMOVQLS -> CMOVQLS
ULE     -> JLS     // different

// ZF == 0 && SF == OF
SETG    -> SETGT   // different
CMOVQGT -> CMOVQGT
GT      -> JGT

// ZF == 1 || SF != OF, good
SETLE   -> SETLE
CMOVQLE -> CMOVQLE
LE      -> JLE

// SF == OF, good
SETGE   -> SETGE
CMOVQGE -> CMOVQGE
GE      -> JGE

// SF != OF
SETL    -> SETLT   // different
CMOVQLT -> CMOVQLT
LT      -> JLT

// CF == 0
SETAE   -> SETCC   // different
CMOVQCC -> CMOVQCC
UGE     -> JCC     // different

// CF == 1
SETB    -> SETCS   // different
CMOVQCS -> CMOVQCS
ULT     -> JCS     // different

// OF == 0
!MISSING!
!MISSING!
OC        -> JOC // unused

// OF == 1
SETO      -> SETOS // different
!MISSING!
OS        -> JOS

// Need different opcodes for floating point conditions because
// any comparison involving a NaN is always FALSE and thus
// the patterns for inverting conditions cannot be used.
// A == B
SETEQF   -> SETEQ
CMOVQEQF -> CMOVQNE // wrong ? Unused anyway
EQF      -> JEQ

// A != B, good
SETNEF   -> SETNE
CMOVQNEF -> CMOVQNE
NEF      -> JNE

// is "ordered" ?
SETORD    -> SETPC // unused
!MISSING!
ORD       -> JPC   // unused

// is NaN ?
SETNAN    -> SETPS
!MISSING!
NAN       -> JPS   // unused

// a > b
SETGF     -> SETHI          // different
CMOVQGTF  -> CMOVQHI
?MISSING? => UGT     -> JHI

// a >= v
SETGEF    -> SETCC
CMOVQGEF  -> CMOVQCC
?MISSING? => UGE     -> JCC
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 13, 2022
@Jorropo
Copy link
Contributor Author

Jorropo commented Nov 13, 2022

Ideally, the generator would know what conditional suffixes are and so:

(If (SETL  cmp) yes no) => (LT  cmp yes no)
(If (SETLE cmp) yes no) => (LE  cmp yes no)
(If (SETG  cmp) yes no) => (GT  cmp yes no)
(If (SETGE cmp) yes no) => (GE  cmp yes no)
(If (SETEQ cmp) yes no) => (EQ  cmp yes no)
(If (SETNE cmp) yes no) => (NE  cmp yes no)
(If (SETB  cmp) yes no) => (ULT cmp yes no)
(If (SETBE cmp) yes no) => (ULE cmp yes no)
(If (SETA  cmp) yes no) => (UGT cmp yes no)
(If (SETAE cmp) yes no) => (UGE cmp yes no)
(If (SETO cmp) yes no) => (OS cmp yes no)

Would be rewritable as:

(If (SETcc cmp) yes no) => (cc cmp yes no)

(this would require some details arround floats, probably have different SETcc and SETFcc, ... to handle NaNs cases)

@gopherbot
Copy link

gopherbot commented Nov 14, 2022

Change https://go.dev/cl/450059 mentions this issue: cmd/compile/ssa/_gen: add support for conditional suffixes opcode generation

@joedian joedian added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 14, 2022
@mknyszek mknyszek added this to the Backlog milestone Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Status: Todo
Development

No branches or pull requests

5 participants