Skip to content

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Jun 11, 2021

This commit tries to propagate constraints imposed on object fields, e.g.:

struct SomeX{T}
    x::Union{Nothing,T}
end
mutable struct MutableSomeX{T}
    const x::Union{Nothing,T}
end

let # o1::SomeX{T}, o2::MutableSomeX{T}
    if !isnothing(o1.x)
        # now inference knows `o1.x::T` here
        ...
        if !isnothing(o2.x)
            # now inference knows `o2.x::T` here
            ...
        end
    end
end

The idea is that we can make isa and === propagate constraint
imposed on an object field if the identity of that object.
We can have such a lattice element that wraps return type of abstract
getfield call together with the object identity, and then we can
form a conditional constraint that propagates the refinement information
imposed on the object field when we see isa/=== applied the return
value of the preceding getfield call.

So this PR defines the new lattice element called MustAlias (and also
InterMustAlias, which just works in a similar way to InterConditional),
which may be formed upon getfield inference to hold the retrieved type
of the field and track the identity of the object (in inference,
"object identity" can be represented as a SlotNumber).
This PR also implements the new logic in abstract_call_builtin so that
isa and === can form a conditional constraint (i.e. Conditional)
from MustAlias-argument that may later refine the wrapped object to
PartialStruct that holds the refined field type information.

One important note here is, MustAlias expects the invariant that the
field of wrapped slot object never changes. The obvious limitation with
this invariant is that it can't propagate constraints imposed on mutable
fields, because inference currently doesn't have a precise (per-object)
knowledge of memory effect.

@aviatesk aviatesk added the compiler:inference Type inference label Jun 11, 2021
@aviatesk
Copy link
Member Author

@femtomc you may also want to have a look at this, since I think this PR illustrates how tightly our abstract interpretation logic is bound to the lattice implementation, currently.

@aviatesk aviatesk changed the title inference: simple back-propagation of constraints on an aliased field inference: simple back-propagation of constraints imposed on aliased field Jun 11, 2021
@aviatesk aviatesk force-pushed the avi/aliasedfieldanalysis branch 2 times, most recently from 844f692 to 6d92655 Compare June 12, 2021 17:05
@aviatesk aviatesk force-pushed the avi/aliasedfieldanalysis branch 2 times, most recently from ce85d62 to d542160 Compare June 24, 2021 16:47
@aviatesk aviatesk force-pushed the avi/aliasedfieldanalysis branch from d542160 to 1bb4a8c Compare July 7, 2021 05:13
@aviatesk
Copy link
Member Author

aviatesk commented Jul 7, 2021

I may first work on an overhaul of our lattice implementation before going with this.

@jakobnissen
Copy link
Member

Will this fix #39888 ?

@aviatesk
Copy link
Member Author

aviatesk commented Jul 9, 2021

It will, assuming Option is immutable.

@aviatesk aviatesk force-pushed the avi/aliasedfieldanalysis branch from f959acd to fe5a54a Compare November 15, 2022 09:20
aviatesk added a commit that referenced this pull request Nov 15, 2022
#41199 implements the base logic of type-based alias
analysis with new lattice element `MustAlias`, but doesn't enable it for
`NativeInterpreter`. This PR enables it for our native code execution
pipeline, checking its performance impact.
@jakobnissen
Copy link
Member

That's fantastic! ❤️ The failure propagate type info to duplicate field loads is indeed one of the main contributors for false positives in JET.
My guess would be that this inference improvement would only improve quality of the generated code a little bit, so it's not worth any significant compiler performance regressions to the average user.
Since it does generate slightly better code, perhaps it would make sense to enable it with -O3 - or does the -O3 flag only affect the backend LLVM compiler? In any case, it's probably not a big deal.

Base automatically changed from avi/lattice-fixes to master November 16, 2022 00:27
@aviatesk aviatesk force-pushed the avi/aliasedfieldanalysis branch from fe5a54a to 6ac900c Compare November 16, 2022 01:19
aviatesk added a commit that referenced this pull request Nov 16, 2022
#41199 implements the base logic of type-based alias
analysis with new lattice element `MustAlias`, but doesn't enable it for
`NativeInterpreter`. This PR enables it for our native code execution
pipeline, checking its performance impact.
@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk aviatesk force-pushed the avi/aliasedfieldanalysis branch from 6ac900c to 374b3f1 Compare November 16, 2022 04:53
@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

aviatesk added a commit that referenced this pull request Nov 16, 2022
#41199 implements the base logic of type-based alias
analysis with new lattice element `MustAlias`, but doesn't enable it for
`NativeInterpreter`. This PR enables it for our native code execution
pipeline, checking its performance impact.
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk
Copy link
Member Author

Yay! This PR introduces some complicated logic into inference, nevertheless it finally comes without any performance cost (actually it even makes it slightly faster)!

This should be ready to go, so I'm planning to merge this.

@vtjnash
Copy link
Member

vtjnash commented Nov 16, 2022

Do you want to do a joint-review tomorrow, or are you content with it as-is for merging?

@aviatesk
Copy link
Member Author

Do you want to do a joint-review tomorrow, or are you content with it as-is for merging?

I'm almost certain that this can be merged as-is (since I confirmed e.g. this PR runs successfully if we enable MustAlias for the native compilation pipeline), while I'm happy to do a joint-review if you want to understand the details of this PR. The basic idea is to propagate MustAlias on getfield(x, :f) call and then form Conditional with PartialStruct when seeing isa/=== conditions on other getfield(x, :f) values.

@DilumAluthge
Copy link
Member

Looks like this needs a manual (command-line) rebase.

…field

This commit tries to propagate constraints imposed on object fields, e.g.:
```julia
struct SomeX{T}
    x::Union{Nothing,T}
end
mutable struct MutableSomeX{T}
    const x::Union{Nothing,T}
end

let # o1::SomeX{T}, o2::MutableSomeX{T}
    if !isnothing(o1.x)
        # now inference knows `o1.x::T` here
        ...
        if !isnothing(o2.x)
            # now inference knows `o2.x::T` here
            ...
        end
    end
end
```

The idea is that we can make `isa` and `===` propagate constraint
imposed on an object field if the _identity_ of that object.
We can have such a lattice element that wraps return type of abstract
`getfield` call together with the object _identity_, and then we can
form a conditional constraint that propagates the refinement information
imposed on the object field when we see `isa`/`===` applied the return
value of the preceding `getfield` call.

So this PR defines the new lattice element called `MustAlias` (and also
`InterMustAlias`, which just works in a similar way to `InterConditional`),
which may be formed upon `getfield` inference to hold the retrieved type
of the field and track the _identity_ of the object (in inference,
"object identity" can be represented as a `SlotNumber`).
This PR also implements the new logic in `abstract_call_builtin` so that
`isa` and `===` can form a conditional constraint (i.e. `Conditional`)
from `MustAlias`-argument that may later refine the wrapped object to
`PartialStruct` that holds the refined field type information.

One important note here is, `MustAlias` expects the invariant that the
field of wrapped slot object never changes. The obvious limitation with
this invariant is that it can't propagate constraints imposed on mutable
fields, because inference currently doesn't have a precise (per-object)
knowledge of memory effect.
@aviatesk aviatesk force-pushed the avi/aliasedfieldanalysis branch from 374b3f1 to a1fa7e3 Compare November 18, 2022 00:29
aviatesk added a commit that referenced this pull request Nov 18, 2022
#41199 implements the base logic of type-based alias
analysis with new lattice element `MustAlias`, but doesn't enable it for
`NativeInterpreter`. This PR enables it for our native code execution
pipeline, checking its performance impact.
@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk
Copy link
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk aviatesk removed the don't squash Don't squash merge label Nov 19, 2022
@aviatesk aviatesk merged commit 1d8f7e0 into master Nov 19, 2022
@aviatesk aviatesk deleted the avi/aliasedfieldanalysis branch November 19, 2022 06:22
aviatesk added a commit that referenced this pull request Nov 19, 2022
#41199 implements the base logic of type-based alias
analysis with new lattice element `MustAlias`, but doesn't enable it for
`NativeInterpreter`. This PR enables it for our native code execution
pipeline, checking its performance impact.
aviatesk added a commit that referenced this pull request Nov 21, 2022
#41199 implements the base logic of type-based alias
analysis with new lattice element `MustAlias`, but doesn't enable it for
`NativeInterpreter`. This PR enables it for our native code execution
pipeline, checking its performance impact.
@vchuravy vchuravy mentioned this pull request Mar 3, 2023
50 tasks
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.

5 participants