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: form PartialStruct for extra type information propagation #42831

Merged
merged 3 commits into from
Nov 1, 2021

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Oct 28, 2021

This commit forms PartialStruct whenever there is any type-level
refinement available about a field, even if it's not "constant" information.

In Julia "definitions" are allowed to be abstract whereas "usages"
(i.e. callsites) are often concrete. The basic idea is to allow inference
to make more use of such precise callsite type information by encoding it
as PartialStruct.

This may increase optimization possibilities of "unidiomatic" Julia code,
which may contain poorly-typed definitions, like this very contrived example:

struct Problem
    n; s; c; t
end

function main(args...)
    prob = Problem(args...)
    s = 0
    for i in 1:prob.n
        m = mod(i, 3)
        s += m == 0 ? sin(prob.s) : m == 1 ? cos(prob.c) : tan(prob.t)
    end
    return prob, s
end

# master: 1ms, this commit: 100μs
main(10000, 1, 2, 3)

One of the obvious limitation is that this extra type information can be
propagated inter-procedurally only as a const-propagation.
I'm not sure this kind of "just a type-level" refinement can often make
constant-prop' successful (i.e. shape-up a method body and allow it to
be inlined, encoding the extra type information into the generated code),
thus I didn't not modify any part of const-prop' heuristics.

So the improvements from this change is almost for local analysis,
and for very simple inter-procedural calls.

Comment on lines 1545 to 1604
local anyconst = anyrefine = false
local allconst = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of adding local in this PR? There is no scope block except the function itself here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the purpose of computing anyconst? It seems to be redundant with computing anyrefine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of adding local in this PR? There is no scope block except the function itself here

I tend to use such local v = rhs declaration just before a loop in order to indicate v may be wrote within the loop (as we need the local v decl if we don't initialize it with rhs). Does it sound weird ? Then I'm welcome to craft a PR to remove these unnecessary local v = rhs decls I recently added.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the purpose of computing anyconst? It seems to be redundant with computing anyrefine

yeah, I unified them into anyrefine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unusual to worry that the scope rules won't work, though not really an issue. I find it can unduly imbalance the code if there are later parallel assignments, but there is nothing either for or against it in the style guide generally

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good find and good improvement!

@aviatesk aviatesk added compiler:inference Type inference merge me PR is reviewed. Merge when all tests are passing labels Oct 29, 2021
@DilumAluthge
Copy link
Member

@aviatesk @vtjnash CI is green, but I saw you are still having a discussion, so I held off on merging, in case you decide to make additional changes.

@aviatesk aviatesk force-pushed the avi/partialtype branch 2 times, most recently from 6bfb3a8 to 3606e59 Compare November 1, 2021 03:08
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Nov 1, 2021
@DilumAluthge
Copy link
Member

I'm removing the merge me label for now, since it seems like you might still be working on this PR.

@aviatesk
Copy link
Member Author

aviatesk commented Nov 1, 2021

No, this PR is solid atm, and I'm going to merge this once CI runs green.

@DilumAluthge DilumAluthge added the merge me PR is reviewed. Merge when all tests are passing label Nov 1, 2021
@DilumAluthge
Copy link
Member

Back it goes :)

This commit forms `PartialStruct` whenever there is any type-level
refinement available about a field, even if it's not "constant" information.

In Julia "definitions" are allowed to be abstract whereas "usages"
(i.e. callsites) are often concrete. The basic idea is to allow inference
to make more use of such precise callsite type information by encoding it
as `PartialStruct`.

This may increase optimization possibilities of "unidiomatic" Julia code,
which may contain poorly-typed definitions, like this very contrived example:
```julia
struct Problem
    n; s; c; t
end

function main(args...)
    prob = Problem(args...)
    s = 0
    for i in 1:prob.n
        m = mod(i, 3)
        s += m == 0 ? sin(prob.s) : m == 1 ? cos(prob.c) : tan(prob.t)
    end
    return prob, s
end

main(10000, 1, 2, 3)
```

One of the obvious limitation is that this extra type information can be
propagated inter-procedurally only as a const-propagation.
I'm not sure this kind of "just a type-level" refinement can often make
constant-prop' successful (i.e. shape-up a method body and allow it to
be inlined, encoding the extra type information into the generated code),
thus I didn't not modify any part of const-prop' heuristics.

So the improvements from this change is almost for local analysis,
and for very simple inter-procedural calls.
@aviatesk aviatesk merged commit a121721 into master Nov 1, 2021
@aviatesk aviatesk deleted the avi/partialtype branch November 1, 2021 10:49
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Nov 1, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
JuliaLang#42831)

* inference: form `PartialStruct` for extra type information propagation

This commit forms `PartialStruct` whenever there is any type-level
refinement available about a field, even if it's not "constant" information.

In Julia "definitions" are allowed to be abstract whereas "usages"
(i.e. callsites) are often concrete. The basic idea is to allow inference
to make more use of such precise callsite type information by encoding it
as `PartialStruct`.

This may increase optimization possibilities of "unidiomatic" Julia code,
which may contain poorly-typed definitions, like this very contrived example:
```julia
struct Problem
    n; s; c; t
end

function main(args...)
    prob = Problem(args...)
    s = 0
    for i in 1:prob.n
        m = mod(i, 3)
        s += m == 0 ? sin(prob.s) : m == 1 ? cos(prob.c) : tan(prob.t)
    end
    return prob, s
end

main(10000, 1, 2, 3)
```

One of the obvious limitation is that this extra type information can be
propagated inter-procedurally only as a const-propagation.
I'm not sure this kind of "just a type-level" refinement can often make
constant-prop' successful (i.e. shape-up a method body and allow it to
be inlined, encoding the extra type information into the generated code),
thus I didn't not modify any part of const-prop' heuristics.

So the improvements from this change might not be very useful for general
inter-procedural analysis currently, but they should definitely improve the
accuracy of local analysis and very simple inter-procedural analysis.
vtjnash added a commit that referenced this pull request Mar 2, 2022
Be more aggressive about merging fields to greatly accelerate
convergence, but also compute anyrefine more correctly as we do now
elsewhere (since #42831, a121721)

Move the tmeet algorithm, without changes, since it is a precise lattice
operation, not a heuristic limit like tmerge.

Close #43784
vtjnash added a commit that referenced this pull request Mar 2, 2022
Be more aggressive about merging fields to greatly accelerate
convergence, but also compute anyrefine more correctly as we do now
elsewhere (since #42831, a121721)

Move the tmeet algorithm, without changes, since it is a precise lattice
operation, not a heuristic limit like tmerge.

Close #43784
vtjnash added a commit that referenced this pull request Mar 2, 2022
Be more aggressive about merging fields to greatly accelerate
convergence, but also compute anyrefine more correctly as we do now
elsewhere (since #42831, a121721)

Move the tmeet algorithm, without changes, since it is a precise lattice
operation, not a heuristic limit like tmerge.

Close #43784
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
JuliaLang#42831)

* inference: form `PartialStruct` for extra type information propagation

This commit forms `PartialStruct` whenever there is any type-level
refinement available about a field, even if it's not "constant" information.

In Julia "definitions" are allowed to be abstract whereas "usages"
(i.e. callsites) are often concrete. The basic idea is to allow inference
to make more use of such precise callsite type information by encoding it
as `PartialStruct`.

This may increase optimization possibilities of "unidiomatic" Julia code,
which may contain poorly-typed definitions, like this very contrived example:
```julia
struct Problem
    n; s; c; t
end

function main(args...)
    prob = Problem(args...)
    s = 0
    for i in 1:prob.n
        m = mod(i, 3)
        s += m == 0 ? sin(prob.s) : m == 1 ? cos(prob.c) : tan(prob.t)
    end
    return prob, s
end

main(10000, 1, 2, 3)
```

One of the obvious limitation is that this extra type information can be
propagated inter-procedurally only as a const-propagation.
I'm not sure this kind of "just a type-level" refinement can often make
constant-prop' successful (i.e. shape-up a method body and allow it to
be inlined, encoding the extra type information into the generated code),
thus I didn't not modify any part of const-prop' heuristics.

So the improvements from this change might not be very useful for general
inter-procedural analysis currently, but they should definitely improve the
accuracy of local analysis and very simple inter-procedural analysis.
vtjnash added a commit that referenced this pull request Mar 10, 2022
Be more aggressive about merging fields to greatly accelerate
convergence, but also compute anyrefine more correctly as we do now
elsewhere (since #42831, a121721)

Move the tmeet algorithm, without changes, since it is a precise lattice
operation, not a heuristic limit like tmerge.

Close #43784
vtjnash added a commit that referenced this pull request Mar 11, 2022
Be more aggressive about merging fields to greatly accelerate
convergence, but also compute anyrefine more correctly as we do now
elsewhere (since #42831, a121721)

Move the tmeet algorithm, without changes, since it is a precise lattice
operation, not a heuristic limit like tmerge.

Close #43784
DilumAluthge pushed a commit that referenced this pull request Mar 13, 2022
* inference: fix tmerge lattice over issimpleenoughtype

Previously we assumed only union type could have complexity that
violated the tmerge lattice requirements, but other types can have that
too. This lets us fix an issue with the PartialStruct comparison failing
for undefined fields, mentioned in #43784.

* inference: refine PartialStruct lattice tmerge

Be more aggressive about merging fields to greatly accelerate
convergence, but also compute anyrefine more correctly as we do now
elsewhere (since #42831, a121721)

Move the tmeet algorithm, without changes, since it is a precise lattice
operation, not a heuristic limit like tmerge.

Close #43784
KristofferC pushed a commit that referenced this pull request Mar 13, 2022
* inference: fix tmerge lattice over issimpleenoughtype

Previously we assumed only union type could have complexity that
violated the tmerge lattice requirements, but other types can have that
too. This lets us fix an issue with the PartialStruct comparison failing
for undefined fields, mentioned in #43784.

* inference: refine PartialStruct lattice tmerge

Be more aggressive about merging fields to greatly accelerate
convergence, but also compute anyrefine more correctly as we do now
elsewhere (since #42831, a121721)

Move the tmeet algorithm, without changes, since it is a precise lattice
operation, not a heuristic limit like tmerge.

Close #43784

(cherry picked from commit ff88fa4)
oscardssmith added a commit that referenced this pull request Jun 23, 2022
improve opaqueclosure inference 
#42831 started calling `tmeet` in abstractinterpretation of `:new` which meant that we now need to be able to infer it.
Co-authored-by: Keno Fischer <[email protected]>
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
improve opaqueclosure inference 
JuliaLang#42831 started calling `tmeet` in abstractinterpretation of `:new` which meant that we now need to be able to infer it.
Co-authored-by: Keno Fischer <[email protected]>
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.

3 participants