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

inference: refine slot undef info within then branch of @isdefined #55545

Merged
merged 1 commit into from
Aug 27, 2024
Merged

inference: refine slot undef info within then branch of @isdefined #55545

merged 1 commit into from
Aug 27, 2024

Conversation

aviatesk
Copy link
Member

By adding some information to Conditional, it is possible to improve the undef information of slot within the then branch of @isdefined slot.
As a result, it's now possible to prove the :nothrow-ness in cases like:

@test Base.infer_effects((Bool,Int)) do c, x
    local val
    if c
        val = x
    end
    if @isdefined val
        return val
    end
    return zero(Int)
end |> Core.Compiler.is_nothrow

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

@aviatesk aviatesk force-pushed the avi/isdefined-nothrow branch from aa8194c to 8483130 Compare August 21, 2024 08:15
@nanosoldier
Copy link
Collaborator

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

Keno
Keno reviewed Aug 21, 2024
View reviewed changes
base/compiler/typelattice.jl
# `isdefined` indicates this `Conditional` is from `@isdefined slot`, implying that
# the `undef` information of `slot` can be improved in the then branch.
# Since this is only beneficial for local inference, it is not translated into `InterConditional`.
isdefined::Bool
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this works conceptually. isdefined-ness is not a flow property and can be reset by NewvarNode

Copy link
Member

Choose a reason for hiding this comment

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

Basically to optimize isdefined you need to do ssa construction

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that is a data-flow property. My understanding was that the type/isdefined-ness information for a slot gets reset by NewvarNode, but then both of those get updated along with the abstract interpretation. E.g. within smerge, both typ and undef are being merged.

The purpose of this PR was to eliminate unnecessary :throw_undef_if_not during SSA construction, but I might be making a fundamental mistake.

Copy link
Member

Choose a reason for hiding this comment

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

Turns out Conditional itself is just broken: #55548

@aviatesk aviatesk force-pushed the avi/isdefined-nothrow branch from 8483130 to 5e7f92d Compare August 22, 2024 06:08
@aviatesk
Copy link
Member Author

@nanosoldier runtests()

@aviatesk
Copy link
Member Author

If the issue is related to Conditional in general, we can address that in a separate PR. As for this PR, I think it should be fine to proceed as long as pkgeval doesn't raise any issues.

@nanosoldier
Copy link
Collaborator

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

@aviatesk aviatesk force-pushed the avi/isdefined-nothrow branch from 5e7f92d to 0fcbc16 Compare August 24, 2024 03:01
@aviatesk
Copy link
Member Author

@nanosoldier runtests(["LinearRationalExpectations", "BenchmarkTools", "MatrixLMnet", "VCFTools", "SSIMLoss", "ThermodynamicIntegration", "DECAES", "Polynomials4ML", "EconomicScenarioGenerators", "MCMCTempering", "DifferentialEvolutionMCMC", "OptimizationOptimJL", "ControlSystemIdentification", "PiecewiseDeterministicMarkovProcesses", "PolynomialGTM", "AstrodynamicalModels", "FaultTolerantControl", "NumCME", "BoxCox", "BifurcationInference", "FourLeafMLE"])

@aviatesk aviatesk force-pushed the avi/isdefined-nothrow branch from 0fcbc16 to db19d0a Compare August 24, 2024 05:09
@nanosoldier
Copy link
Collaborator

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

@aviatesk
inference: refine slot undef info within then branch of @isdefined
3b9dc4b 
By adding some information to `Conditional`, it is possible to improve the
`undef` information of `slot` within the `then` branch of
`@isdefined slot`.
As a result, it's now possible to prove the `:nothrow`-ness in cases like:
```julia
@test Base.infer_effects((Bool,Int)) do c, x
    local val
    if c
        val = x
    end
    if @isdefined val
        return val
    end
    return zero(Int)
end |> Core.Compiler.is_nothrow
```
@aviatesk aviatesk force-pushed the avi/isdefined-nothrow branch from db19d0a to 3b9dc4b Compare August 26, 2024 07:24
@aviatesk
Copy link
Member Author

pkgeval looks just fine, so I believe this PR should be fine to proceed as is, while any issues related to Conditional itself should be discussed separately in #55551.

@aviatesk aviatesk merged commit 78b0b74 into master Aug 27, 2024
7 checks passed
@aviatesk aviatesk deleted the avi/isdefined-nothrow branch August 27, 2024 12:29
aviatesk added a commit that referenced this pull request Aug 27, 2024
@aviatesk
optimizer: don't insert :throw_undef_if_not for defined slots
e945dbd 
As an application of #55545, this commit avoids the
insertion of `:throw_undef_if_not` nodes when the defined-ness of a
slot is guaranteed by abstract interpretation.

```julia
julia> function isdefined_nothrow(c, x)
           local val
           if c
               val = x
           end
           if @isdefined val
               return val
           end
           return zero(Int)
       end;

julia> @code_typed isdefined_nothrow(true, 42)
```
```diff
diff --git a/old b/new
index c4980a5c9c..3d1d6d30f0 100644
--- a/old
+++ b/new
@@ -4,7 +4,6 @@ CodeInfo(
 3 ┄ %3 = φ (#2 => x, #1 => #undef)::Int64
 │   %4 = φ (#2 => true, #1 => false)::Bool
 └──      goto #5 if not %4
-4 ─      $(Expr(:throw_undef_if_not, :val, :(%4)))::Any
-└──      return %3
+4 ─      return %3
 5 ─      return 0
 ) => Int64
```
@aviatesk aviatesk mentioned this pull request Aug 27, 2024
Merged
udesou pushed a commit to udesou/julia that referenced this pull request Aug 28, 2024
@aviatesk @udesou
inference: refine slot undef info within then branch of @isdefined (
5963dc0 
JuliaLang#55545)

By adding some information to `Conditional`, it is possible to improve
the `undef` information of `slot` within the `then` branch of
`@isdefined slot`.
As a result, it's now possible to prove the `:nothrow`-ness in cases
like:
```julia
@test Base.infer_effects((Bool,Int)) do c, x
    local val
    if c
        val = x
    end
    if @isdefined val
        return val
    end
    return zero(Int)
end |> Core.Compiler.is_nothrow
```
aviatesk added a commit that referenced this pull request Aug 29, 2024
@aviatesk
optimizer: don't insert :throw_undef_if_not for defined slots (#55600)
ac0161a 
As an application of #55545, this commit avoids the
insertion of `:throw_undef_if_not` nodes when the defined-ness of a slot
is guaranteed by abstract interpretation.

```julia
julia> function isdefined_nothrow(c, x)
           local val
           if c
               val = x
           end
           if @isdefined val
               return val
           end
           return zero(Int)
       end;

julia> @code_typed isdefined_nothrow(true, 42)
```
```diff
diff --git a/old b/new
index c4980a5c9c..3d1d6d30f0 100644
--- a/old
+++ b/new
@@ -4,7 +4,6 @@ CodeInfo(
 3 ┄ %3 = φ (#2 => x, #1 => #undef)::Int64
 │   %4 = φ (#2 => true, #1 => false)::Bool
 └──      goto #5 if not %4
-4 ─      $(Expr(:throw_undef_if_not, :val, :(%4)))::Any
-└──      return %3
+4 ─      return %3
 5 ─      return 0
 ) => Int64
```
KristofferC pushed a commit that referenced this pull request Sep 12, 2024
@aviatesk
inference: refine slot undef info within then branch of @isdefined (
8146235 
#55545)

By adding some information to `Conditional`, it is possible to improve
the `undef` information of `slot` within the `then` branch of
`@isdefined slot`.
As a result, it's now possible to prove the `:nothrow`-ness in cases
like:
```julia
@test Base.infer_effects((Bool,Int)) do c, x
    local val
    if c
        val = x
    end
    if @isdefined val
        return val
    end
    return zero(Int)
end |> Core.Compiler.is_nothrow
```
KristofferC pushed a commit that referenced this pull request Sep 12, 2024
@aviatesk
optimizer: don't insert :throw_undef_if_not for defined slots (#55600)
32480cd 
As an application of #55545, this commit avoids the
insertion of `:throw_undef_if_not` nodes when the defined-ness of a slot
is guaranteed by abstract interpretation.

```julia
julia> function isdefined_nothrow(c, x)
           local val
           if c
               val = x
           end
           if @isdefined val
               return val
           end
           return zero(Int)
       end;

julia> @code_typed isdefined_nothrow(true, 42)
```
```diff
diff --git a/old b/new
index c4980a5c9c..3d1d6d30f0 100644
--- a/old
+++ b/new
@@ -4,7 +4,6 @@ CodeInfo(
 3 ┄ %3 = φ (#2 => x, #1 => #undef)::Int64
 │   %4 = φ (#2 => true, #1 => false)::Bool
 └──      goto #5 if not %4
-4 ─      $(Expr(:throw_undef_if_not, :val, :(%4)))::Any
-└──      return %3
+4 ─      return %3
 5 ─      return 0
 ) => Int64
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Keno Keno Keno left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@aviatesk @nanosoldier @Keno