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

inference: Remove special casing for ! #55271

Merged
merged 1 commit into from
Aug 1, 2024
Merged

inference: Remove special casing for ! #55271

merged 1 commit into from
Aug 1, 2024

Conversation

Keno
Copy link
Member

@Keno Keno commented Jul 26, 2024

We have a handful of cases in inference where we look up functions by name (using istopfunction) and give them special behavior. I'd like to remove these. They're not only aesthetically ugly, but because they depend on binding lookups, rather than values, they have unclear semantics as those bindings change. They are also unsound should a user use the same name for something differnt in their own top modules (of course, it's unlikely that a user would do such a thing, but it's bad that they can't).

This particular PR removes the special case for !, which was there to strengthen the inference result for ! on Conditional. However, with a little bit of strengthening of the rest of the system, this can be equally well evaluated through the existing InterConditional mechanism.

@Keno Keno requested a review from aviatesk July 26, 2024 23:06
@Keno Keno added the needs pkgeval Tests for all registered packages should be run with this change label Jul 26, 2024
@giordano giordano added the compiler:inference Type inference label Jul 26, 2024
@Keno
Copy link
Member Author

Keno commented Jul 28, 2024

@nanosoldier runtests()

Keno added a commit that referenced this pull request Jul 28, 2024
As mentioned in #55271, I'd like to get rid of `istopfunction` for various reasons.
Rather than hardcoding constprop overrides based on binding name, this annotates
the relevant TypeNames (like the existing `max_methods` override), removing the
binding lookups from inference.
Keno added a commit that referenced this pull request Jul 28, 2024
As mentioned in #55271, I'd like to get rid of `istopfunction` for various reasons.
Rather than hardcoding constprop overrides based on binding name, this annotates
the relevant TypeNames (like the existing `max_methods` override), removing the
binding lookups from inference.
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.
@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Keno added a commit that referenced this pull request Jul 30, 2024
…#55288)

As mentioned in #55271, I'd like to get rid of `istopfunction` for
various reasons. Rather than hardcoding constprop overrides based on
binding name, this annotates the relevant TypeNames (like the existing
`max_methods` override), removing the binding lookups from inference.
@Keno
Copy link
Member Author

Keno commented Jul 31, 2024

@nanosoldier runtests(["UnitTestDesign", "EarlyStopping", "QuantumAlgebra", "Miter", "BaytesFilters", "UnitDiskMapping", "DiffEqFinancial", "ReachabilityAnalysis", "UltraDark", "FSimBase", "HydroPowerSimulations", "PiecewiseDeterministicMarkovProcesses", "FiniteStateProjection"])

We have a handful of cases in inference where we look up functions by name
(using `istopfunction`) and give them special behavior. I'd like to remove
these. They're not only aesthetically ugly, but because they depend on binding
lookups, rather than values, they have unclear semantics as those bindings
change. They are also unsound should a user use the same name for something
different in their own top modules (of course, it's unlikely that a user would
do such a thing, but it's bad that they can't).

This particular PR removes the special case for `!`, which was there to
strengthen the inference result for `!` on Conditional. However, with
a little bit of strengthening of the rest of the system, this can be
equally well evaluated through the existing InterConditional mechanism.
@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@Keno Keno removed the needs pkgeval Tests for all registered packages should be run with this change label Aug 1, 2024
@Keno Keno merged commit 7c6a1a2 into master Aug 1, 2024
7 of 8 checks passed
@Keno Keno deleted the kf/nospecialbang branch August 1, 2024 13:47
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
…JuliaLang#55288)

As mentioned in JuliaLang#55271, I'd like to get rid of `istopfunction` for
various reasons. Rather than hardcoding constprop overrides based on
binding name, this annotates the relevant TypeNames (like the existing
`max_methods` override), removing the binding lookups from inference.
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
We have a handful of cases in inference where we look up functions by
name (using `istopfunction`) and give them special behavior. I'd like to
remove these. They're not only aesthetically ugly, but because they
depend on binding lookups, rather than values, they have unclear
semantics as those bindings change. They are also unsound should a user
use the same name for something differnt in their own top modules (of
course, it's unlikely that a user would do such a thing, but it's bad
that they can't).

This particular PR removes the special case for `!`, which was there to
strengthen the inference result for `!` on Conditional. However, with a
little bit of strengthening of the rest of the system, this can be
equally well evaluated through the existing InterConditional mechanism.
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
Keno added a commit that referenced this pull request Oct 23, 2024
This commit teaches to compiler to update its world bounds whenever
it looks at a binding partition, making the compiler sound in the
presence of a partitioned binding. The key adjustment is that the
compiler is no longer allowed to directly query the binding table
without recording the world bounds, so all the various abstract
evaluations that look at bindings need to be adjusted and are no
longer pure tfuncs. We used to look at bindings a lot more, but
thanks to earlier prep work to remove unnecessary binding-dependent
code (#55288, #55289 and #55271), these changes become relatively
straightforward.

Note that as before, we do not create any binding partitions by
default, so this commit is mostly preperatory.
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 this pull request may close these issues.

4 participants