Skip to content

Conversation

Keno
Copy link
Member

@Keno Keno commented Jul 26, 2024

This was introduced in [1] to improve constant propagation. These days this is handled via concrete eval and the special case is no longer required.

[1] 0292c42

This was introduced in [1] to improve constant propagation.
These days this is handled via concrete eval and the special
case is no longer required.

[1] 0292c42
@Keno
Copy link
Member Author

Keno commented Jul 26, 2024

There's a remaining question here of whether we're losing any precision for the ::Type case and if so whether that matters.

@Keno
Copy link
Member Author

Keno commented Jul 26, 2024

We do lose this one:

julia> f(x) = Base.typename(x)
f (generic function with 1 method)

Before:

1-element Vector{Any}:
 CodeInfo(
1 ─     return $(QuoteNode(typename(Array)))
) => Core.TypeName

After:

julia> code_typed(f, Tuple{Type{Union{Vector{Int64}, Vector{Float64}}}})
1-element Vector{Any}:
 CodeInfo(
1 ─ %1 = invoke Base.typename(x::Union)::Any
└──      return %1
) => Any

Let me think about whether there's something that can be tweaked here with effect annotations.

@Keno
Copy link
Member Author

Keno commented Jul 26, 2024

If not, the easy answer is just to put this one in Core and compare it by value, which doesn't have the same problems.

@Keno
Copy link
Member Author

Keno commented Jul 27, 2024

If not, the easy answer is just to put this one in Core and compare it by value, which doesn't have the same problems.

I think I'll probably just do this. The key extra bit of information that the compiler does not know is that checking that they're all equal retroactively makes taking union components out legal, but I don't see a good generic way to annotate that.

@giordano giordano added the compiler:inference Type inference label Jul 27, 2024
Keno added a commit that referenced this pull request Jul 28, 2024
As mentioned in #55271, the `istopfunction` binding-based comparisons
are problematic. In #55272 and #55273, I attempted to remove the inference
special cases for `>:` and `typename` (respectively) entirely, but for
differing reasons (`>:` gets too many extra specializations, `typename`
loses precision), those PRs are suboptimal. As discussed in #55273, this
PR instead moves these functions to Core, so that both `Core.Compiler`
and `Base` share the function object, allowing inference to detect them
and apply the special handling by simple value-comparison.
Keno added a commit that referenced this pull request Jul 28, 2024
As mentioned in #55271, the `istopfunction` binding-based comparisons
are problematic. In #55272 and #55273, I attempted to remove the inference
special cases for `>:` and `typename` (respectively) entirely, but for
differing reasons (`>:` gets too many extra specializations, `typename`
loses precision), those PRs are suboptimal. As discussed in #55273, this
PR instead moves these functions to Core, so that both `Core.Compiler`
and `Base` share the function object, allowing inference to detect them
and apply the special handling by simple value-comparison.
Keno added a commit that referenced this pull request Jul 28, 2024
As mentioned in #55271, the `istopfunction` binding-based comparisons
are problematic. In #55272 and #55273, I attempted to remove the inference
special cases for `>:` and `typename` (respectively) entirely, but for
differing reasons (`>:` gets too many extra specializations, `typename`
loses precision), those PRs are suboptimal. As discussed in #55273, this
PR instead moves these functions to Core, so that both `Core.Compiler`
and `Base` share the function object, allowing inference to detect them
and apply the special handling by simple value-comparison.
@aviatesk
Copy link
Member

This PR is going to be replaced by #55289 .

@aviatesk aviatesk added the DO NOT MERGE Do not merge this PR! label Jul 29, 2024
Keno added a commit that referenced this pull request Aug 1, 2024
…alue

As mentioned in #55271, the `istopfunction` binding-based comparisons
are problematic. In #55272 and #55273, I attempted to remove the inference
special cases for `>:` and `typename` (respectively) entirely, but for
differing reasons (`>:` gets too many extra specializations, `typename`
loses precision), those PRs are suboptimal. As discussed in #55273, this
PR instead moves these functions to Core, so that both `Core.Compiler`
and `Base` share the function object, allowing inference to detect them
and apply the special handling by simple value-comparison.
Keno added a commit that referenced this pull request Aug 2, 2024
…55289)

As mentioned in #55271, the `istopfunction` binding-based comparisons
are problematic. In #55272 and #55273, I attempted to remove the
inference special cases for `>:` and `typename` (respectively) entirely,
but for differing reasons (`>:` gets too many extra specializations,
`typename` loses precision), those PRs are suboptimal. As discussed in
#55273, this PR instead moves these functions to Core, so that both
`Core.Compiler` and `Base` share the function object, allowing inference
to detect them and apply the special handling by simple
value-comparison.

---

- closes #55273
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
…uliaLang#55289)

As mentioned in JuliaLang#55271, the `istopfunction` binding-based comparisons
are problematic. In JuliaLang#55272 and JuliaLang#55273, I attempted to remove the
inference special cases for `>:` and `typename` (respectively) entirely,
but for differing reasons (`>:` gets too many extra specializations,
`typename` loses precision), those PRs are suboptimal. As discussed in
JuliaLang#55273, this PR instead moves these functions to Core, so that both
`Core.Compiler` and `Base` share the function object, allowing inference
to detect them and apply the special handling by simple
value-comparison.

---

- closes JuliaLang#55273
@giordano giordano deleted the kf/nospecialtypename branch October 7, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference DO NOT MERGE Do not merge this PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants