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

remove special case for homogeneous tuples in all and any #47454

Merged
merged 2 commits into from
Nov 5, 2022
Merged

Conversation

KristofferC
Copy link
Sponsor Member

These were added in #44063 but seem to cause some ambiguities, for example in Nullables.jl:

  MethodError: all(::typeof(Nullables.hasvalue), ::Tuple{Nullable{Int64}, Nullable{Int64}}) is ambiguous.
  
  Candidates:
    all(f, itr::Tuple{Vararg{T, N}} where {N, T})
      @ Base reduce.jl:1298
    all(f::typeof(Nullables.hasvalue), t::Tuple)
      @ Nullables ~/.julia/packages/Nullables/RNaHb/src/nullable.jl:371
  
  Possible fix, define
    all(::typeof(Nullables.hasvalue), ::Tuple{Vararg{T, N}} where {N, T})

This is a pretty weird overload from Nullables but It's unclear to me if there is any benefit of having homogenous tuples fall back to the loop version.

cc @jipolanco

@jipolanco
Copy link
Contributor

Ok for me. But maybe we should also ask @N5N3 who actually suggested the special case: #44063 (comment).

I guess another solution would be to add an extra dispatch layer, something like:

any(f, itr::Tuple) = _any(f, itr)

_any(f, itr::NTuple) = _any(f, itr, :)  # case of homogeneous tuple

function _any(f, itr::Tuple)            # case of tuple with mixed types
    length(itr) > 32 && return _any(f, itr, :)
    _any_tuple(f, false, itr...)
end

Unroll this loop at julia level wont gain any inference improvement, thus let LLVM unroll it if needed.
@N5N3
Copy link
Member

N5N3 commented Nov 5, 2022

If there is any benefit of having homogenous tuples fall back to the loop version.

As homogenous tuples, unroll the loop at julia level wont gain any inference improvement thus I think we'd better let LLVM do this optimization for us and save some compile overhead.

@Seelengrab
Copy link
Contributor

I think this may be the same issue as #47385?

@N5N3 N5N3 merged commit 27859f3 into master Nov 5, 2022
@N5N3 N5N3 deleted the kc/ambig branch November 5, 2022 06:04
@N5N3
Copy link
Member

N5N3 commented Nov 5, 2022

NTuple is not a Union type so probably not?

@Seelengrab
Copy link
Contributor

Maybe 🤔 It depends on the specifics of dispatch in this case, but it at least feels similar enough 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants