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

Incorrect bounds check elimination #18261

Closed
yuyichao opened this issue Aug 27, 2016 · 3 comments · Fixed by #22826
Closed

Incorrect bounds check elimination #18261

yuyichao opened this issue Aug 27, 2016 · 3 comments · Fixed by #22826
Labels
compiler:inference Type inference

Comments

@yuyichao
Copy link
Contributor

yuyichao commented Aug 27, 2016

IIUC, @inbounds should only affect one level of inlining unless propagate_inbounds is specified, however, due to how inlining works, this is currently not true in certain cases.

julia> @inline f(a) = a[1]
f (generic function with 1 method)

julia> @inline g(a) = f(a)
g (generic function with 1 method)

julia> function k(a)
           @inbounds x = g(a)
           return x
       end
k (generic function with 1 method)

julia> @code_warntype k(Int[])
Variables:
  #self#::#k
  a::Array{Int64,1}
  x::Int64

Body:
  begin 
      $(Expr(:inbounds, true))
      x::Int64 = (Base.arrayref)(a::Array{Int64,1},1)::Int64
      $(Expr(:inbounds, :pop)) # line 3:
      return x::Int64
  end::Int64

julia> k(Int[])
0

The issue here is that the Expr(:return) is not included when adding the inbounds metadata during inlining. A simpler case where f(a) and g(a) are not oneliner and have a simpler return statement is also currently wrong on master but is fixed in #18260 (see the added test).

Maybe we can just wait for fully linear IR?

@blakejohnson
Copy link
Contributor

I'll take a look at this next week.

@yuyichao yuyichao added the compiler:inference Type inference label Aug 27, 2016
@vtjnash
Copy link
Member

vtjnash commented Aug 30, 2016

this is basically just a special case of #9765

@yuyichao
Copy link
Contributor Author

Is it? The issue here is that we always need to inline

return foo(...)

as

Expr(:inbounds, false)
SSAValue(n) = foo(...)
Expr(:inbounds, :pop)
return SSAValue(n)

Unless foo is guaranteed to be not affected by boundscheck status. I don't think the effect_free check is wrong here (or is used at all).

yuyichao added a commit to yuyichao/BaseBenchmarks.jl that referenced this issue Aug 19, 2017
Currently the benchmarks are relying on inlining bugs to propagate boundscheck elision
but the bug will be fixed with linear IR.

Ref JuliaLang/julia#18261
This is affecting JuliaLang/julia#23240
ararslan pushed a commit to JuliaCI/BaseBenchmarks.jl that referenced this issue Aug 21, 2017
Currently the benchmarks are relying on inlining bugs to propagate boundscheck elision
but the bug will be fixed with linear IR.

Ref JuliaLang/julia#18261
This is affecting JuliaLang/julia#23240
yuyichao added a commit that referenced this issue Nov 5, 2017
These were previously relying on bug in the inliner. (#18261)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants