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

Jump to bottom

cmd/compile: rematerializable ops must not clobber flags #21080

Closed
mundaym opened this issue Jul 19, 2017 · 5 comments
Closed

cmd/compile: rematerializable ops must not clobber flags #21080

mundaym opened this issue Jul 19, 2017 · 5 comments
Labels
FrozenDueToAge

Comments

@mundaym
Copy link
Member

mundaym commented Jul 19, 2017

While debugging #21048 I noticed that there was a conditional jump immediately after a MOVDaddr instruction. This shouldn't happen because MOVDaddr clobbers flags on s390x.

Digging deeper it looks to me like flagalloc is silently failing in runtime.gcDrain. The problem is reproducible on darwin/amd64.

If you do:

GOSSAFUNC=gcDrain go install -a runtime

And look at the generated ssa.html after the regalloc phase you'll see this:

b3: ← b1
    v17 = LoweredGetG <*g> v1 : AX
    v20 = MOVQload <*m> [48] v17 v1 : AX
    v23 = MOVQload <*g> [152] v20 v1 : AX
    v122 = StoreReg <*g> v23 : gp[*g]
    v142 = LoadReg <*gcWork> v6 : CX
    v41 = MOVQload <int64> [24] v142 v1 : DX
    v46 = MOVLload <uint32> {"".work} [136] v3 v1 : BX
    v49 = MOVLload <uint32> {"".work} [140] v3 v1 : SI
    v187 = LoadReg <gcDrainFlags> v7 : DI
    v18 = BTLconst <flags> [0] v187    <===== v18 SET
    v356 = BTLconst <flags> [2] v187   <===== v18 CLOBBERED
    v234 = BTLconst <flags> [3] v187
    v197 = CMPL <flags> v46 v49
ULT v197 → b4 b48

b4: ← b3
    v204 = StoreReg <int64> v41 : initScanWork[int64]
    v100 = BTLconst <flags> [0] v187
Plain → b6

b6: ← b4 b8
    v365 = Phi <mem> v1 v366
ULT v18 → b10 b40                      <==== v18 USED

v18 is clobbered immediately. v356 and v234 are generated but immediately clobbered. All these BTL instructions make it into the final assembly.
@mundaym
Copy link
Member Author

mundaym commented Jul 19, 2017

/cc @randall77 @dr2chase @cherrymui

@mundaym
Copy link
Member Author

mundaym commented Jul 19, 2017

Ah, looking through the flagalloc code I see this is the expected behavior because the flags are regenerated as necessary, but the values aren't hooked up.

My problem is actually the regalloc pass inserting MOVDaddr ops after the flagalloc pass. I guess it assumes the op doesn't clobber flags. Probably the right fix is to try and make MOVDaddr not clobber flags on s390x somehow (currently it can insert an ADD with 32-bit immediate in shared mode, hence the clobber flags mark).

I suspect that can wait until 1.10. It's probably also worth trying to get rid of the redundant instructions, not sure if there is already an issue for that?

@mundaym mundaym changed the title cmd/compile: multiple flags values live in runtime.gcDrain cmd/compile: MOVDaddr op on s390x shouldn't clobber flags Jul 19, 2017
@cherrymui
Copy link
Member

Yeah, the register allocator may insert MOVDaddr because it is "rematerializable". But if it may clobber flags, it is not really rematerializable. Marking MOVDaddr not rematerializable probably makes liveness unhappy, though.

Is there an instruction that does the address calculation without clobbering flags, like LEA on 386? I tried to change ADD $n, Rx to MOVD $n(Rx), Rx in rewriteToUseGot, but it didn't work (seg fault). Apparently I missed something...

@mundaym
Copy link
Member Author

mundaym commented Jul 19, 2017

Yeah, we have LAY, but it has a couple of limitations so it's not quite a straight swap. Firstly it can't take R0 as in input, but we could easily workaround that by removing R0 from the list of valid MOVDaddr targets. The second issue is that the offset is limited to a 20-bit signed integer which means we need to use a temp register when the offset is larger.

@gopherbot
Copy link

Change https://golang.org/cl/63030 mentions this issue: cmd/compile: stop MOVDaddr clobbering flags on s390x

@mundaym mundaym changed the title cmd/compile: MOVDaddr op on s390x shouldn't clobber flags cmd/compile: rematerializable ops must not clobber flags Sep 13, 2017
@golang golang locked and limited conversation to collaborators Sep 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gopherbot @cherrymui @mundaym