RFC: Change lowering of destructuring to avoid const prop dependence #37268
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| :g, :h, :i, :j, :k, :l, :m, :n, :o, :p, :q, :r, :s, :t, :u, :v, :w, :x, :y, :z, :add_int, | ||
| :sub_int, :mul_int, :add_float, :sub_float, :new, :mul_float, :bitcast, :start, :done, :next, | ||
| :indexed_iterate, :getfield, :meta, :eq_int, :slt_int, :sle_int, :ne_int, :push_loc, :pop_loc, | ||
| :index_and_iterate, :getfield, :meta, :eq_int, :slt_int, :sle_int, :ne_int, :push_loc, :pop_loc, |
Member
There was a problem hiding this comment.
To be super conservative, we should leave this here and replace a reserved slot with the new symbol instead. Of course, loading code with indexed_iterate won't work anyway, but at least it's slightly more compatible.
Member
|
👍 to the concept. Shouldn't it be called |
base/missing.jl
Outdated
| convert(::Type{T}, x) where {T>:Missing} = convert(nonmissingtype_checked(T), x) | ||
| convert(::Type{T}, x) where {T>:Union{Missing, Nothing}} = convert(nonmissingtype_checked(nonnothingtype_checked(T)), x) | ||
|
|
||
| index_and_iterate(::Missing) = throw(MethodError(iterate, (missing,))) |
Member
There was a problem hiding this comment.
I assume this is there for the same reason as the one for nothing, but would it make sense to add a comment here as well?
I'm currently doing some work with inference passes that have const
prop (temporarily) disabled and I noticed we actually rely on it
quite a bit for basic things. That's not terrible - const prop works
pretty well after all, but it still imposes a cost and while I want
to support it in my AD use case also, it makes destructuring quite
expensive, because everything needs to be inferred twice. This PR
is an experiment in changing the lowering to avoid having to const
prop the index. Rather than lowering `(a,b,c) = foo()` as:
```
it = foo()
a, s = indexed_iterate(it, 1)
b, s = indexed_iterate(it, 2)
c, s = indexed_iterate(it, 3)
```
we lower as:
```
it = foo()
iterate, index = iterate_and_index(it)
x = iterate(it)
a = index(x, 1)
y = iterate(it, y)
b = index(y, 2)
z = iterate(it, z)
c = index(z, 3)
```
For tuples `iterate` would simply return the first argument and
`index` would be `getfield`. That way, there is no const prop,
since `getfield` is called directly and inference can directly
use its tfunc. For the fallback case `iterate` is basically
just `Base.iterate`, with just a slight tweak to give an intelligent
error for short iterables.
On simple functions, there isn't much of a difference in execution
time, but benchmarking something more complicated like:
```
function g()
a, = getfield(((1,),(2.0,3),("x",),(:x,)), Base.inferencebarrier(1))
nothing
end
```
shows about a 20% improvement in end-to-end inference/optimize time,
which is substantial.
Keno
added a commit
that referenced
this pull request
Aug 30, 2020
So after thinking about #37268, I was wondering what would happen if one applied the same trick to the lowering of `getproperty` as well. This PR starts at that, but is mostly a placeholder for discussion of whether this is something we want to do. In particular, this PR lowers `a.b` to: ``` getproperty(a)(a, :b) ``` with a default implementation of ``` getproperty(a) = getfield ``` The inference benefit is as expected. Timing end-to-end inference&optimize time of: ``` struct Foo; end f(a::Foo) = a.x ``` shows that time to infer `f` drops by about a third (because it doesn't need to infer getproperty twice). Overall build time of the system image drops by 6% for me. If we do decide to go this way, things that would still need to be fixed: - [] Do the same for `setproperty!` - [] `ccall` doesn't like this lowering. Didn't look into it too closely - [] Needs a fallback to backwards compatibility. I'm thinking something along the lines of ``` getproperty(x) = hasmethod(getproperty, Tuple{typeof(x), Symbol}) ? getproperty : getfield ``` but obviously it would need inference integration to be fast.
3 tasks
cirocavani
reviewed
Aug 30, 2020
Co-authored-by: Ciro Cavani <[email protected]>
Member
|
Bump. Is this good to go? |
Member
|
Just bumping this again. 🙂 |
Member
Author
|
We decided we may want to get rid of special lowering for destructuring, so we decided against this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm currently doing some work with inference passes that have const
prop (temporarily) disabled and I noticed we actually rely on it
quite a bit for basic things. That's not terrible - const prop works
pretty well after all, but it still imposes a cost and while I want
to support it in my AD use case also, it makes destructuring quite
expensive, because everything needs to be inferred twice. This PR
is an experiment in changing the lowering to avoid having to const
prop the index. Rather than lowering
(a,b,c) = foo()as:we lower as:
For tuples
iteratewould simply return the first argument andindexwould begetfield. That way, there is no const prop,since
getfieldis called directly and inference can directlyuse its tfunc. For the fallback case
iterateis basicallyjust
Base.iterate, with just a slight tweak to give an intelligenterror for short iterables.
On simple functions, there isn't much of a difference in execution
time, but benchmarking something more complicated like:
shows about a 20% improvement in end-to-end inference/optimize time,
which is substantial.