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.

Sign up for GitHub

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

Jump to bottom

[compiler] Remove ConstAPI struct #48598

Merged
merged 7 commits into from
Feb 10, 2023
Merged

[compiler] Remove ConstAPI struct #48598

merged 7 commits into from
Feb 10, 2023

Conversation

staticfloat
Copy link
Sponsor Member

ConstAPI predates the effect analysis framework, and was relying on an
ad-hoc implementation of effect analysis that only worked for functions
of less than 15 statements. As a result, it wasn't used much anymore,
but it could cause trouble with external AbstractInterpreters that may
want to cache additional code metadata within CodeInstances.

This commit simply removes this, expecting that the existing effect
analysis will pick up the slack.

aviatesk
aviatesk reviewed Feb 9, 2023
View reviewed changes
base/compiler/typeinfer.jl Outdated Show resolved Hide resolved
base/compiler/typeinfer.jl Outdated Show resolved Hide resolved
@staticfloat
Copy link
Sponsor Member Author

I've added a change to try and fix the .cov test failure, and while it improves the IR, we're still not getting a .cov file.

@staticfloat staticfloat marked this pull request as draft February 9, 2023 17:23
N5N3 and others added 6 commits February 9, 2023 22:33
@N5N3 @staticfloat
Avoid split y::Union when comparing with a forall var.
f9c9f9a 
If `y` has no free typevar. Then `x <:  y` wont cause any env change. Thus there's no need to split `y`.
@staticfloat
[compiler] Remove ConstAPI struct
28cf6f9 
`ConstAPI` predates the effect analysis framework, and was relying on an
ad-hoc implementation of effect analysis that only worked for functions
of less than 15 statements.  As a result, it wasn't used much anymore,
but it could cause trouble with external `AbstractInterpreters` that may
want to cache additional code metadata within `CodeInstance`s.

This commit simply removes this, expecting that the existing effect
analysis will pick up the slack.
@staticfloat
Add short-circuit has_offset_axes(::Array)
c06d1d0 
This ensures that `Base.require_one_based_indexing()` is always fully
eliminated, since previously `has_offset_axes(::Array)` is
effect-tainted as non-consistent due to calculating `size(::Array)`.
This dodges the effect taint by adding this short-circuit, fixing broken
tests due to using effects analysis more extensively.
@staticfloat
Address review comments
8e22073
@staticfloat
Move coverage effects analysis into inference
f353814 
Code coverage is implemented as an optimization pass that inserts an SSA
IR node during optimization, however effects analysis is perfomed during
inference, and so the desired effects tainting that should occur from
insertion of the code coverage IR node is ignored.

This teaches `InferenceState` to taint itself with the effects flag
 `effects_free = ALWAYS_FALSE` when coverage is applicable to this
inference state's method.
@staticfloat
Try to fix the tests
5e94b86 
We just assume that you don't want code coverage if you're using this
constructor. This may not always be true.
@staticfloat
Copy link
Sponsor Member Author

I believe I have fixed the last bit of tests related to code coverage.

Code coverage is implemented by inserting a special code coverage IR node into the IR, however it is inserted at optimization time, which is after inference is finished assigning effects to return types. To fix this, we moved the code coverage check (including scoped coverage selection) into inference time, mark each InferenceFrame as needing a coverage node insertion or not, and then taint the effects as "not effect free". This correctly prevents the coverage nodes from being dropped by later optimization passes, and brings back the coverage we all know and love.

@staticfloat
Copy link
Sponsor Member Author

staticfloat commented Feb 9, 2023 •
edited
Loading

One remaining question is the second constructor to OptimizationState; it does not contain a reference to its InferenceState, so we can't receive the insert_coverage boolean; because the core compiler does not use this constructor, I assume it's okay to just hardcode insert_coverage as false here; but if not, I'd appreciate guidance on what to do here instead.

X-ref: 5e94b86

@oscardssmith oscardssmith marked this pull request as ready for review February 9, 2023 23:46
@oscardssmith oscardssmith merged commit ce292c1 into master Feb 10, 2023
@oscardssmith oscardssmith deleted the sf/ConstAPI_must_die branch February 10, 2023 02:32
aviatesk added a commit that referenced this pull request Feb 10, 2023
@aviatesk
NFC followups on #48598
47d981e 
- added missing `is_inlineable_constant` check in the inlining code of `InferenceResult`
- rename `is_total` to `is_foldable_nothrow` (since `:total` subsumes
  the other program properties too now)
- updated docstring of `Core.Compiler.finish`
aviatesk added a commit that referenced this pull request Feb 10, 2023
@aviatesk
NFC followups on #48598
0b2768a 
- added missing `is_inlineable_constant` check in the inlining code of `InferenceResult`
- rename `is_total` to `is_foldable_nothrow` (since `:total` subsumes
  the other program properties too now)
- updated docstring of `Core.Compiler.finish`
aviatesk added a commit that referenced this pull request Feb 10, 2023
@aviatesk
NFC followups on #48598
98d744b 
- added missing `is_inlineable_constant` check in the inlining code of `InferenceResult`
- rename `is_total` to `is_foldable_nothrow` (since `:total` subsumes
  the other program properties too now)
- updated docstring of `Core.Compiler.finish`
aviatesk added a commit that referenced this pull request Feb 10, 2023
@aviatesk
NFC followups on #48598
d7fd1fe 
- added missing `is_inlineable_constant` check in the inlining code of `InferenceResult`
- rename `is_total` to `is_foldable_nothrow` (since `:total` subsumes
  the other program properties too now)
- updated docstring of `Core.Compiler.finish`
aviatesk added a commit that referenced this pull request Feb 10, 2023
@aviatesk
NFC followups on #48598 (#48622)
28e0f46 
- added missing `is_inlineable_constant` check in the inlining code of `InferenceResult`
- rename `is_total` to `is_foldable_nothrow` (since `:total` subsumes
  the other program properties too now)
- updated docstring of `Core.Compiler.finish`
aviatesk added a commit that referenced this pull request Feb 10, 2023
@aviatesk
allow external AbstractInterpreter to keep source of const-folded c…
b2fb881 
…ache

After #48598, the `CodeInstance` constructor discards `inferred_result`
when the cache can use the constant calling convention.
This is generally fine, but external `AbstractInterpreter` may still want
to keep the discarded source, that may be arbitrary custom data
structure. This commit makes use of the `may_discard_trees` interface to
configure that behavior.
@aviatesk aviatesk mentioned this pull request Feb 10, 2023
Merged
aviatesk added a commit that referenced this pull request Feb 10, 2023
@aviatesk
allow external AbstractInterpreter to keep source of const-folded c…
a44b576 
…ache (#48626)

After #48598, the `CodeInstance` constructor discards `inferred_result`
when the cache can use the constant calling convention.
This is generally fine, but external `AbstractInterpreter` may still want
to keep the discarded source, that may be arbitrary custom data
structure. This commit makes use of the `may_discard_trees` interface to
configure that behavior.
@Seelengrab Seelengrab mentioned this pull request May 28, 2023
Open
@Seelengrab Seelengrab mentioned this pull request Jul 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aviatesk aviatesk aviatesk left review comments

@Keno Keno Keno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@staticfloat @Keno @aviatesk @oscardssmith @N5N3