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

Side-effectful foreach callback mysteriously eliminated on nightly (miscompilation?) #48807

Closed
ToucheSir opened this issue Feb 27, 2023 · 2 comments · Fixed by #48826
Closed
Labels
bug Indicates an unexpected problem or unintended behavior compiler:effects effect analysis
Milestone

Comments

@ToucheSir
Copy link

ToucheSir commented Feb 27, 2023

I discovered this while trying to determine why Flux's nightly CI was failing. Have only managed to reduce it to the following:

using ChainRulesCore

function mwe(x, v::Val{S}) where {S}
    function inner(ys)
        l = size(x, 1)
        dx = fill!(similar(x, float(eltype(x)), l * S, size(x, 2)), false)
        gs = ntuple(v) do n
            @view dx[begin+(l*(n-1)):(l*n), :]
        end
        foreach(gs, ys) do g, y
          y isa AbstractZero && return
          copy!(g, y)
        end
        # Works
        # for (g, y) in zip(gs, ys)
        #   y isa AbstractZero && continue
        #   copy!(g, y)
        # end
        return dx
    end
    inner
end

x = zeros(2, 3)
f = mwe(x, Val(3))
ys = (a=ones(2, 3), b=ZeroTangent(), c=ZeroTangent())
f(ys)

Expected output:

6×3 Matrix{Float64}:
 1.0  1.0  1.0
 1.0  1.0  1.0
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

Actual output:

6×3 Matrix{Float64}:
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

What makes this mysterious is what seemingly minor changes can restore the expected behaviour:

  • Using the loop instead of foreach
  • Defining a custom abstract type + 2 zero-sized struct subtypes mirroring ChainRulesCore's AbstractZero + NoTangent/ZeroTangent. This only seems to work with the CRC types.
  • Using a Vector instead of a NamedTuple. Note that the original example used ChainRulesCore.Tangent, but I'm not sure if that indicates anything.
  • Adding Core.donotdelete(false) in the foreach callback. This suggests to me that the call to said callback is being completely eliminated somehow. Unfortunately Cthulhu won't let me descend into the Core._apply_iterate(Base.iterate, f, z) call that would actually call said callback, so the best I have to go on here is the following remarks:
Call result type was widened because the return value is unused [constprop]
Disabled by argument and rettype heuristics [constprop] Disabled by entry heuristic (unimprovable result)

Versioninfo:

Julia Version 1.10.0-DEV.661
Commit 0b8e8fc216a (2023-02-25 23:01 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × Intel(R) Core(TM) i7-4790K CPU @ 4.00GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, haswell)
  Threads: 1 on 8 virtual cores
@oscardssmith oscardssmith added bug Indicates an unexpected problem or unintended behavior compiler:effects effect analysis labels Feb 27, 2023
@KristofferC KristofferC added this to the 1.10 milestone Feb 27, 2023
@inkydragon
Copy link
Member

  • ❌ Version 1.10.0-DEV.667 (2023-02-27) a23b29c
  • ❌ Version 1.10.0-DEV.180 (2022-12-22) a074d06
  • ✅ Version 1.10.0-DEV.179 (2022-12-22) 181328c
  • ✅ Version 1.10.0-DEV.0 (2022-11-14) 7fe6b16
  • ✅ Julia Version 1.9.0-beta4
  • ✅ Version 1.8.5 (2023-01-08)

Note: No need to build it yourself, you can download pre-built artifacts in CI.

> git bisect good
a074d06e2d8d645fca6c6751325f6f31bda99a2f is the first bad commit
commit a074d06e2d8d645fca6c6751325f6f31bda99a2f
Author: Shuhei Kadowaki <[email protected]>
Date:   Thu Dec 22 20:30:28 2022 +0900

    inference: mark flag for effect-free `:call`s during abstractinterpret (#47689)

    So that they can be deleted during the first `compact!`-ion.
    This allows us to delete an inlineable and effect-free, but unused call.

    This is essentially an alternative of #47305, but doesn't introduce a
    problem like #47374.

 base/compiler/abstractinterpretation.jl | 15 +++++++++++++++
 base/compiler/inferencestate.jl         |  2 ++
 test/compiler/inline.jl                 | 23 +++++++++++++++++++++--
 3 files changed, 38 insertions(+), 2 deletions(-)

a074d06 cc: @aviatesk

@aviatesk
Copy link
Member

Thanks so much for the bisect, @inkydragon !

aviatesk added a commit that referenced this issue Feb 28, 2023
We need to make sure that we widen both type and effects when bailing
out inference. I wanted to make a simplified test case for this, but
it turns out to be a bit tricky since it relies on the detail of union
split and the bailing out logic.

fix #48807
aviatesk added a commit that referenced this issue Mar 1, 2023
Since we allow overloading of the `bail_out_xxx` hooks, we need to make
sure that we widen both type and effects to the top when bailing on
inference regardless of the condition presumed by a hook.

This commit particularly fixes the correctness of `bail_out_apply`
(fixes #48807). I wanted to make a simplified test case for this, but
it turns out to be a bit tricky since it relies on the details of
multiple match analysis and the bail out logic.
aviatesk added a commit that referenced this issue Mar 1, 2023
Since we allow overloading of the `bail_out_xxx` hooks, we need to make
sure that we widen both type and effects to the top when bailing on
inference regardless of the condition presumed by a hook.

This commit particularly fixes the correctness of `bail_out_apply`
(fixes #48807). I wanted to make a simplified test case for this, but
it turns out to be a bit tricky since it relies on the details of
multiple match analysis and the bail out logic.
aviatesk added a commit that referenced this issue Mar 1, 2023
Since we allow overloading of the `bail_out_xxx` hooks, we need to make
sure that we widen both type and effects to the top when bailing on
inference regardless of the condition presumed by a hook.

This commit particularly fixes the correctness of `bail_out_apply`
(fixes #48807). I wanted to make a simplified test case for this, but
it turns out to be a bit tricky since it relies on the details of
multiple match analysis and the bail out logic.
aviatesk added a commit that referenced this issue Mar 1, 2023
Since we allow overloading of the `bail_out_xxx` hooks, we need to make
sure that we widen both type and effects to the top when bailing on
inference regardless of the condition presumed by a hook.

This commit particularly fixes the correctness of `bail_out_apply`
(fixes #48807). I wanted to make a simplified test case for this, but
it turns out to be a bit tricky since it relies on the details of
multiple match analysis and the bail out logic.
KristofferC pushed a commit that referenced this issue Mar 3, 2023
Since we allow overloading of the `bail_out_xxx` hooks, we need to make
sure that we widen both type and effects to the top when bailing on
inference regardless of the condition presumed by a hook.

This commit particularly fixes the correctness of `bail_out_apply`
(fixes #48807). I wanted to make a simplified test case for this, but
it turns out to be a bit tricky since it relies on the details of
multiple match analysis and the bail out logic.
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:effects effect analysis
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants