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: unnecessary compares following FP instructions in ppc64le #17507

Closed
laboger opened this issue Oct 18, 2016 · 4 comments
Closed

cmd/compile: unnecessary compares following FP instructions in ppc64le #17507

laboger opened this issue Oct 18, 2016 · 4 comments

Comments

@laboger
Copy link
Contributor

laboger commented Oct 18, 2016

Please answer these questions before submitting your issue. Thanks!

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

tip

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

Ubuntu 16.04

What did you do?

Looking for performance improvements to the function math.mod when run as part of the math package testcase (math.test).

What did you see instead?

I'm seeing some unusual and unnecessary compare type instructions that are following floating point compare instructions when the compare is gt or lt. The eq and ne appear good.

I'm seeing code like this:

 7f5a0:       00 20 03 fc     fcmpu   cr0,f3,f4
 7f5a4:       01 00 e0 3b     li      r31,1
 7f5a8:       5e 00 7f 7c     iselgt  r3,r31,r0
 7f5ac:       00 00 03 7c     cmpw    r3,r0

Should be just bgt or blt after the fcmpu.

If the compare following the fcmpu is beq or bne then it avoids the isel like this:

7f58c:       00 10 01 fc     fcmpu   cr0,f1,f2
7f590:       60 01 82 41     beq     7f6f0 <math.mod+0x190>
@laboger
Copy link
Contributor Author

laboger commented Oct 18, 2016

Here is the output when GOSSAFUNC is set:

The GT case:

 v100 = LoadReg <float64> v7 : F3
 v117 = FMOVDconst <float64> [1.7976931348623157e+308] : F4
 v81 = FCMPU <flags> v100 v117
 v17 = FGreaterThan <bool> v81 : R3
 v83 = CMPWconst <flags> [0] v17
 NE v83 -> b32 b12

The NE case:

v18 = FCMPU <flags> v100 v100
NE v18 -> b21 b17 (unlikely)

@dr2chase dr2chase self-assigned this Oct 19, 2016
@laboger
Copy link
Contributor Author

laboger commented Oct 19, 2016

Found these lines commented out in the PPC64.rules so I changed them:

-// (NE (CMPWconst [0](FLessThan cc)) yes no) -> (FLT cc yes no)
-// (NE (CMPWconst [0](FLessEqual cc)) yes no) -> (FLE cc yes no)
-// (NE (CMPWconst [0](FGreaterThan cc)) yes no) -> (FGT cc yes no)
-// (NE (CMPWconst [0](FGreaterEqual cc)) yes no) -> (FGE cc yes no)
+(NE (CMPWconst [0](FLessThan cc)) yes no) -> (FLT cc yes no)
+(NE (CMPWconst [0](FLessEqual cc)) yes no) -> (FLE cc yes no)
+(NE (CMPWconst [0](FGreaterThan cc)) yes no) -> (FGT cc yes no)
+(NE (CMPWconst [0](FGreaterEqual cc)) yes no) -> (FGE cc yes no)

That seemed to fix the case I show above but in this function I see some of this:

b15: <- b13
v18 = FCMPU v7 v7
v29 = NotEqual v18 DEAD
v20 = CMPWconst [0] v29 DEAD
NE v18 -> b21 b17
b17: <- b15
v88 = FCMPU v8 v8
v33 = NotEqual v88 DEAD
v14 = CMPWconst [0] v33 DEAD
NE v88 -> b21 b3

v18 05850 (/home/boger/golang/base/go/src/math/mod.go:24) FCMPU F3, F3
b15 05851 (/home/boger/golang/base/go/src/math/mod.go:24) BNE 5906
v88 05852 (/home/boger/golang/base/go/src/math/mod.go:24) FCMPU F1, F1
b17 05853 (/home/boger/golang/base/go/src/math/mod.go:24) BNE 5906

@dr2chase
Copy link
Contributor

dr2chase commented Oct 19, 2016

We need to be sure that's an actual optimizer glitch, and not code that is checking that either operand is a NaN. And guess what:

    if y == 0 || IsInf(x, 0) || IsNaN(x) || IsNaN(y) {
        return NaN()
    }

I assigned myself the bug, if you want to do the uncommenting CL that works too.

@gopherbot
Copy link

CL https://golang.org/cl/31612 mentions this issue.

@golang golang locked and limited conversation to collaborators Oct 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants