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: function inlining produces incorrect results in certain conditions [1.12 backport] #30567

Closed
gopherbot opened this issue Mar 4, 2019 · 4 comments

Comments

Projects
None yet
4 participants
@gopherbot
Copy link

commented Mar 4, 2019

@mvdan requested issue #30566 to be considered for backport to the next 1.12 minor release.

@gopherbot please backport to 1.12.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Mar 6, 2019

Change https://golang.org/cl/165858 mentions this issue: [release-branch.go1.12] cmd/compile: fix ordering for short-circuiting ops

@julieqiu

This comment has been minimized.

Copy link

commented Mar 12, 2019

@mvdan - there isn't a reason provided in the gopherbot message. Would you mind providing one for this backport?

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

There's an underlying bug which is fixed with the CL linked above. That bug has been in existence since at least 1.2.
However, with temporary variable reuse introduced in 1.12, it now causes actual incorrect results more often. So I think it qualifies for a backport.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Mar 13, 2019

Closed by merging f062f48 to release-branch.go1.12.

@gopherbot gopherbot closed this Mar 13, 2019

gopherbot pushed a commit that referenced this issue Mar 13, 2019

[release-branch.go1.12] cmd/compile: fix ordering for short-circuitin…
…g ops

Make sure the side effects inside short-circuited operations (&& and ||)
happen correctly.

Before this CL, we attached the side effects to the node itself using
exprInPlace. That caused other side effects in sibling expressions
to get reordered with respect to the short circuit side effect.

Instead, rewrite a && b like:

r := a
if r {
  r = b
}

That code we can keep correctly ordered with respect to other
side-effects extracted from part of a big expression.

exprInPlace seems generally unsafe. But this was the only case where
exprInPlace is called not at the top level of an expression, so I
don't think the other uses can actually trigger an issue (there can't
be a sibling expression). TODO: maybe those cases don't need "in
place", and we can retire that function generally.

This CL needed a small tweak to the SSA generation of OIF so that the
short circuit optimization still triggers. The short circuit optimization
looks for triangle but not diamonds, so don't bother allocating a block
if it will be empty.

Go 1 benchmarks are in the noise.

Fixes #30567

Change-Id: I19c04296bea63cbd6ad05f87a63b005029123610
Reviewed-on: https://go-review.googlesource.com/c/go/+/165617
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit 4a9064e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/165858
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.