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: ICE due to "Flag* ops should never make it to codegen" #22198

Closed
mdempsky opened this issue Oct 10, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@mdempsky
Copy link
Member

commented Oct 10, 2017

$ go build -gcflags=-l=4 k8s.io/kubernetes/vendor/github.com/golang/protobuf/proto
# k8s.io/kubernetes/vendor/github.com/golang/protobuf/proto
../../../go/src/k8s.io/kubernetes/vendor/github.com/golang/protobuf/proto/text.go:344:27: internal compiler error: Flag* ops should never make it to codegen v1291 = FlagEQ <flags>

/cc @randall77

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2017

Not surprising, but it also repros with the original github.com/golang/protobuf/proto repo.

@mdempsky mdempsky changed the title cmd/compile: ICE building kubernetes with -l=4 cmd/compile: ICE building github.com/golang/protobuf/proto with -l=4 Oct 10, 2017

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2017

The seemingly relevant SSA ops are:

v1291 = FlagEQ <flags>
v932 = LoadReg <*bool> v2089 : DX
v794 = SETEQmem <mem> v932 v1291 v789

These are generated by lower from:

v449 = Const8 <byte> [10] (c[byte], c[byte], c[byte], c[byte], c[byte], c[byte])
v791 = Eq8 <bool> v449 v449
v794 = Store <mem> {bool} v742 v791 v789

I'm guessing the rewrite rules just need reordering so that we instead lower into moving constant 1 to memory.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2017

Adding a rule for

(SETEQmem [off] {sym} ptr (FlagEQ) mem) -> (MOVBstore [off] {sym} ptr (MOVLconst [1]) mem)

seems to fix the ICE. Not sure if that's the best solution. (A complete fix would at least handle all the other SETCC/FlagCC combinations.)

/cc @cherrymui

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2017

Seems a regression from CL https://go-review.googlesource.com/c/go/+/67950. Indeed the handling of constant flags is missing. Adding @mdempsky's rule looks right to me.

cc @TocarIP

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2017

all the other SETCC/FlagCC combinations.

There are already rules (around line 1487 of AMD64.rules) that handles non-mem version of SETCC and constant flags. Seems we should add the similar ones for SETCCmem.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2017

Here's the best I've been able to minimize the test case so far:

package p

func f(a *bool, b bool) {
        if b {
                return
        }
        c := '\n'
        if b {
                c = ' '
        }
        *a = c == '\n'
}
@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2017

Also, the repro happens even without -l=4. It just happens to have been discovered in the protobuf code using -l=4.

@mdempsky mdempsky changed the title cmd/compile: ICE building github.com/golang/protobuf/proto with -l=4 cmd/compile: ICE due to "Flag* ops should never make it to codegen" Oct 10, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Oct 11, 2017

Change https://golang.org/cl/69990 mentions this issue: cmd/compile: fold constant comparisions into SETxxmem ops.

@gopherbot gopherbot closed this in 624630b Oct 11, 2017

@golang golang locked and limited conversation to collaborators Oct 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.