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

Possibly incorrect inference through isequal #42457

Closed
Sacha0 opened this issue Oct 1, 2021 · 3 comments · Fixed by #42686
Closed

Possibly incorrect inference through isequal #42457

Sacha0 opened this issue Oct 1, 2021 · 3 comments · Fixed by #42686
Labels
bug Indicates an unexpected problem or unintended behavior compiler:inference Type inference
Milestone

Comments

@Sacha0
Copy link
Member

Sacha0 commented Oct 1, 2021

On 1.6.3

               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.6.3 (2021-09-23)
 _/ |\__'_|_|_|\__'_|  |
|__/                   |

julia> function fluffs(a::Tuple{Int,Int,Int}, b::Tuple)::Bool
           return Base.isequal(a, Base.inferencebarrier(b)::Tuple)
       end
fluffs (generic function with 1 method)

julia> fluffs((1, 1, 1), (1, 1, 1))
true

julia> @code_typed optimize=false fluffs((1, 1, 1), (1, 1, 1))
CodeInfo(
1 ─ %1 = Main.Bool::Core.Const(Bool)
│   %2 = Base.isequal::Core.Const(isequal)
│   %3 = Base.inferencebarrier::Core.Const(Base.inferencebarrier)
│   %4 = (%3)(b)::Any
│   %5 = Core.typeassert(%4, Main.Tuple)::Tuple
│   %6 = (%2)(a, %5)::Any
│   %7 = Base.convert(%1, %6)::Any
│   %8 = Core.typeassert(%7, %1)::Bool
└──      return %8
) => Bool

whereas on 1.7-RC1 and master

               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.8.0-DEV.632 (2021-10-01)
 _/ |\__'_|_|_|\__'_|  |  Commit df52b9409b (0 days old master)
|__/                   |

julia> function fluffs(a::Tuple{Int,Int,Int}, b::Tuple)::Bool
           return Base.isequal(a, Base.inferencebarrier(b)::Tuple)
       end
fluffs (generic function with 1 method)

julia> fluffs((1, 1, 1), (1, 1, 1))
false

julia> @code_typed optimize=false fluffs((1, 1, 1), (1, 1, 1))
CodeInfo(
1 ─ %1 = Main.Bool::Core.Const(Bool)
│   %2 = Base.isequal::Core.Const(isequal)
│   %3 = Base.inferencebarrier::Core.Const(Base.inferencebarrier)
│   %4 = (%3)(b)::Any
│   %5 = Core.typeassert(%4, Main.Tuple)::Tuple
│   %6 = (%2)(a, %5)::Core.Const(false)
│   %7 = Base.convert(%1, %6)::Core.Const(false)
│   %8 = Core.typeassert(%7, %1)::Core.Const(false)
└──      return %8
) => Bool

Notice that the isequal call has ::Core.Const(false) in the latter, which seems to be incorrect.

Interestingly, that still holds after dropping the ::Bool from the method signature, but

julia> function fluffs(a::Tuple{Int,Int,Int}, b::Tuple)
           return Base.isequal(a, Base.inferencebarrier(b)::Tuple)
       end
fluffs (generic function with 1 method)

julia> fluffs((1, 1, 1), (1, 1, 1))
true

julia> @code_typed optimize=false fluffs((1, 1, 1), (1, 1, 1))
CodeInfo(
1 ─ %1 = Base.isequal::Core.Const(isequal)
│   %2 = Base.inferencebarrier::Core.Const(Base.inferencebarrier)
│   %3 = (%2)(b)::Any
│   %4 = Core.typeassert(%3, Main.Tuple)::Tuple
│   %5 = (%1)(a, %4)::Core.Const(false)
└──      return %5
) => Bool

Note that using the new definition for Base.isequal from 1.7-RC1/master on 1.6.3 yields the same issue:

               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.6.3 (2021-09-23)
 _/ |\__'_|_|_|\__'_|  |
|__/                   |

julia> begin
           new_isequal(t1::Tuple, t2::Tuple) = length(t1) == length(t2) && _new_isequal(t1, t2)
           _new_isequal(::Tuple{}, ::Tuple{}) = true
           function _new_isequal(t1::Tuple{Any,Vararg{Any,N}}, t2::Tuple{Any,Vararg{Any,N}}) where {N}
               isequal(t1[1], t2[1]) || return false
               t1, t2 = Base.tail(t1), Base.tail(t2)
               # avoid dynamic dispatch by telling the compiler relational invariants
               return isa(t1, Tuple{}) ? true : _new_isequal(t1, t2::Tuple{Any,Vararg{Any}})
           end
       end
_new_isequal (generic function with 2 methods)

julia> function fluffs(a::Tuple{Int,Int,Int}, b::Tuple)::Bool
           return new_isequal(a, Base.inferencebarrier(b)::Tuple)
       end
fluffs (generic function with 1 method)

julia> fluffs((1, 1, 1), (1, 1, 1))
false

julia> @code_typed optimize=false fluffs((1, 1, 1), (1, 1, 1))
CodeInfo(
1 ─ %1 = Main.Bool::Core.Const(Bool)
│   %2 = Base.inferencebarrier::Core.Const(Base.inferencebarrier)
│   %3 = (%2)(b)::Any
│   %4 = Core.typeassert(%3, Main.Tuple)::Tuple
│   %5 = Main.new_isequal(a, %4)::Core.Const(false)
│   %6 = Base.convert(%1, %5)::Core.Const(false)
│   %7 = Core.typeassert(%6, %1)::Core.Const(false)
└──      return %7
) => Bool

The new definition seems ok though, which suggests to me that the issue is in inference on 1.6.3, 1.7-RC1, and master. Best! :)

@Sacha0 Sacha0 added bug Indicates an unexpected problem or unintended behavior compiler:inference Type inference labels Oct 1, 2021
@vtjnash
Copy link
Member

vtjnash commented Oct 1, 2021

I wonder if this is related to #39098

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 5, 2021

#40594 similarly transformed the definitions of _isless, __inc, and __dec, which may also be impacted.

@vchuravy vchuravy added this to the 1.7 milestone Oct 9, 2021
@martinholters
Copy link
Member

I wonder if this is related to #39098

Looks like it, as inferring fluffs eventually leads to inferring _isequal(Tuple{Any, Vararg{Any, N}}, Tuple{Any, Vararg{Any, N}}) where {N} for argument types (Tuple{Int64, Int64, Int64}, Tuple{Any, Any, Any, Any}) which don't actually match the signature. That in turn is caused by

julia> typeintersect(Tuple{Tuple{Any, Vararg{Any, N}}, Tuple{Any, Vararg{Any, N}}} where N, Tuple{Tuple{Int,Int,Int}, Tuple})
Tuple{Tuple{Int64, Int64, Int64}, NTuple{4, Any}}

which looks pretty similar to #39088 (comment).

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:inference Type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants