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

loop fails to increment counter #30594

Closed
twistingedge opened this issue Jan 4, 2019 · 4 comments
Closed

loop fails to increment counter #30594

twistingedge opened this issue Jan 4, 2019 · 4 comments
Labels
bug Indicates an unexpected problem or unintended behavior compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)

Comments

@twistingedge
Copy link

twistingedge commented Jan 4, 2019

When running a simple for loop, the contents of a counter changes depending on the presence (and location) of a println() in the loop. If the println() is above the line modifying the counter, the line modifying the counter has no effect (optimized out?), and the counter's value does not change during the loop. If the println() is below the modification, the variable changes as expected. I have an earlier (and larger) example that passes the counter to a different (non-println()) function below the modification, and that also produces the error.

This behavior reproduces in 1.0.1, 0.7.0, and RC 1.1. This behavior does not reproduce in 0.6.2. (All versions checked were generic 64-bit binaries under Ubuntu 18.04.1 LTS.)

Note there are two fixes to the issue: add a println() after the variable increment inside the loop, or change the '+' method so it does not use copy().

import Base: +, copy

mutable struct DataWrap
    x::Float64
end # DataWrap
copy(x::DataWrap) = DataWrap(x.x)

function add!(p::DataWrap, off::DataWrap)
    p.x += off.x
    return p
end # add! DataWrap

+(a::DataWrap, b::DataWrap) = add!(copy(a), b) # resolve dispatch
# if you use the version below, the error clears up
# +(a::DataWrap, b::DataWrap) = DataWrap(a.x + b.x)

function put_colonnade(cnt::Int, dx::Int, dy::Int)
    step = DataWrap(dx)
    curr = step + DataWrap(1)
    for i in 1:cnt
        println(curr)
        curr = curr + step
# if you uncomment the line below, the variable increments as expected
#        println(curr)
    end
end

put_colonnade(4, -1,  0)

sample bad output:

julia> include("loopbug0.jl")
DataWrap(0.0)
DataWrap(0.0)
DataWrap(0.0)
DataWrap(0.0)

sample good output (with second println() uncommented):

julia> include("loopbug0.jl")
DataWrap(0.0)
DataWrap(-1.0)
DataWrap(-1.0)
DataWrap(-2.0)
DataWrap(-2.0)
DataWrap(-3.0)
DataWrap(-3.0)
DataWrap(-4.0)
@twistingedge twistingedge changed the title loop loop fails to increment counter Jan 4, 2019
@JeffBezanson
Copy link
Sponsor Member

Seems like this could be an optimizer issue. @Keno ?

@Keno
Copy link
Member

Keno commented Jan 4, 2019

yeah looks like a bug in mutable struct sroa maybe?

@Keno Keno added bug Indicates an unexpected problem or unintended behavior compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) labels Jan 4, 2019
Keno added a commit that referenced this issue Jan 5, 2019
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.
Keno added a commit that referenced this issue Jan 5, 2019
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.
Keno added a commit that referenced this issue Jan 6, 2019
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.
KristofferC pushed a commit that referenced this issue Jan 9, 2019
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.

(cherry picked from commit f8f2045)
KristofferC pushed a commit that referenced this issue Jan 11, 2019
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.

(cherry picked from commit f8f2045)
@JeffBezanson
Copy link
Sponsor Member

Fixed by #30598 (?)

@Keno
Copy link
Member

Keno commented Jan 12, 2019

yes

KristofferC pushed a commit that referenced this issue Feb 4, 2019
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.

(cherry picked from commit f8f2045)
KristofferC pushed a commit that referenced this issue Feb 4, 2019
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.

(cherry picked from commit f8f2045)
KristofferC pushed a commit that referenced this issue Feb 11, 2019
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.

(cherry picked from commit f8f2045)
KristofferC pushed a commit that referenced this issue Feb 11, 2019
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.

(cherry picked from commit f8f2045)
KristofferC pushed a commit that referenced this issue Apr 20, 2019
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.

(cherry picked from commit f8f2045)
KristofferC pushed a commit that referenced this issue Feb 20, 2020
PR #28478 moved the computation of the use counts before the finish call.
to fix #28444. However, the early parts of the finish call fixes up phi
node arguments, which fail to get counted if we look at use counts before
that fixup is performed. This causes #30594 where the only non-trivial use
is on the backedge of the phi and would thus incorrectly fail to get accounted
for. Fix that by taking the use count after phi fixup but before dce.

(cherry picked from commit f8f2045)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

No branches or pull requests

3 participants