inference: introduce more error/throwness checks for array primitives #43587

aviatesk · 2021-12-29T12:02:56Z

Obvious errors are usually caught in higher places like convert, but
it's better to have error checks at each builtin level in order to enable
early bail out from errorneous code compilation (when somehow it does not
rely on common abstractions). These checks are also useful for third
consumers like JET.

KristofferC · 2021-12-29T16:37:17Z

base/compiler/tfuncs.jl

+    ary ⊑ Array || return false
+    if isa(dim, Const)
+        dimval = dim.val
+        return isa(dimval, Int) && dimval > 0


Does an OOMError count? Probably not.

I don't think arraysize can yield OOMError though? AFAIU array resizing may result in OOMError, but all them are invoked by foreigncall, which currently is handled conservatively i.e. considered as "may throw" always.

Yes, I read a bit too quickly and thought it was about array construction. Might have mixed it up with some other PR.

yeah array constructions are handled by #43565, and it does account for OOMError.

aviatesk · 2021-12-29T18:31:15Z

base/compiler/optimize.jl

 const _PURE_OR_ERROR_BUILTINS = [
    fieldtype, apply_type, isa, UnionAll,
-    getfield, arrayref, const_arrayref, isdefined, Core.sizeof,
+    getfield, arrayref, const_arrayref, arraysize, isdefined, Core.sizeof,


~~Hm now I think arraysize may not be eligible to be "pure" function since it is volatile.~~ nvm, it's valid to eliminate such calls as far as their result aren't used anywhere and guaranteed not to throw.

In a similar spirit to #43587, this commit introduces error check for `setfield!`. We can bail out from inference if we can prove either of: - the object is not mutable type - the object is `Module` object - the value being assigned is incompatible with the declared type of object field This commit also adds the throwness check for `setfield!` (i.e. `setfield!_nothrow`). This throwness check won't be used in the current native compilation pipeline since `setfield!` call can't be eliminated even if we can prove that it never throws. But this throwness check would be used by EscapeAnalysis.jl integration and so I'd like to include it in Base.

In a similar spirit to JuliaLang#43587, this commit introduces error check for `setfield!`. We can bail out from inference if we can prove either of: - the object is not mutable type - the object is `Module` object - the value being assigned is incompatible with the declared type of object field This commit also adds the throwness check for `setfield!` (i.e. `setfield!_nothrow`). This throwness check won't be used in the current native compilation pipeline since `setfield!` call can't be eliminated even if we can prove that it never throws. But this throwness check would be used by EscapeAnalysis.jl integration and so I'd like to include it in Base.

In a similar spirit to #43587, this commit introduces error check for `setfield!`. We can bail out from inference if we can prove either of: - the object is not mutable type - the object is `Module` object - the value being assigned is incompatible with the declared type of object field This commit also adds the throwness check for `setfield!` (i.e. `setfield!_nothrow`). This throwness check won't be used in the current native compilation pipeline since `setfield!` call can't be eliminated even if we can prove that it never throws. But this throwness check would be used by EscapeAnalysis.jl integration and so I'd like to include it in Base.

base/compiler/tfuncs.jl

vtjnash · 2022-01-07T17:50:41Z

base/compiler/tfuncs.jl

+    hasintersect(widenconst(boundcheck), Bool) || return false
+    hasintersect(widenconst(ary), Array) || return false
+    for i = 1:length(idxs)
+        idx = idxs[i]


Suggested change

idx = idxs[i]

idx = getfield(idxs, i)

Why don't we want to rely on getindex abstraction here?

base/compiler/tfuncs.jl

vtjnash

LGTM

In a similar spirit to #43587, this commit introduces error check for `setfield!`. We can bail out from inference if we can prove either of: - the object is not mutable type - the object is `Module` object - the value being assigned is incompatible with the declared type of object field This commit also adds the throwness check for `setfield!` (i.e. `setfield!_nothrow`). This throwness check won't be used in the current native compilation pipeline since `setfield!` call can't be eliminated even if we can prove that it never throws. But this throwness check would be used by EscapeAnalysis.jl integration and so I'd like to include it in Base.

Obvious errors are usually caught in higher places like `convert`, but it's better to have error checks at each builtin level in order to enable early bail out from errorneous code compilation (when somehow it does not rely on common abstraction). These checks are also useful for third consumers like JET.

Co-authored-by: Jameson Nash <[email protected]>

In a similar spirit to #43587, this commit introduces error check for `setfield!`. We can bail out from inference if we can prove either of: - the object is not mutable type - the object is `Module` object - the value being assigned is incompatible with the declared type of object field This commit also adds the throwness check for `setfield!` (i.e. `setfield!_nothrow`). This throwness check won't be used in the current native compilation pipeline since `setfield!` call can't be eliminated even if we can prove that it never throws. But this throwness check would be used by EscapeAnalysis.jl integration and so I'd like to include it in Base.

In a similar spirit to JuliaLang#43587, this commit introduces error check for `setfield!`. We can bail out from inference if we can prove either of: - the object is not mutable type - the object is `Module` object - the value being assigned is incompatible with the declared type of object field This commit also adds the throwness check for `setfield!` (i.e. `setfield!_nothrow`). This throwness check won't be used in the current native compilation pipeline since `setfield!` call can't be eliminated even if we can prove that it never throws. But this throwness check would be used by EscapeAnalysis.jl integration and so I'd like to include it in Base.

…JuliaLang#43587) Obvious errors are usually caught in higher places like `convert`, but it's better to have error checks at each builtin level in order to enable early bail out from errorneous code compilation (when somehow it does not rely on common abstraction). These checks are also useful for third consumers like JET.

In a similar spirit to JuliaLang#43587, this commit introduces error check for `setfield!`. We can bail out from inference if we can prove either of: - the object is not mutable type - the object is `Module` object - the value being assigned is incompatible with the declared type of object field This commit also adds the throwness check for `setfield!` (i.e. `setfield!_nothrow`). This throwness check won't be used in the current native compilation pipeline since `setfield!` call can't be eliminated even if we can prove that it never throws. But this throwness check would be used by EscapeAnalysis.jl integration and so I'd like to include it in Base.

…JuliaLang#43587) Obvious errors are usually caught in higher places like `convert`, but it's better to have error checks at each builtin level in order to enable early bail out from errorneous code compilation (when somehow it does not rely on common abstraction). These checks are also useful for third consumers like JET.

aviatesk added the compiler:inference Type inference label Dec 29, 2021

aviatesk force-pushed the avi/arrayerrorcheck branch from 9e9add7 to 2ec059a Compare December 29, 2021 15:09

aviatesk changed the title ~~inference: introduce error checks for array primitives~~ inference: introduce more error/throwness checks for array primitives Dec 29, 2021

aviatesk requested a review from vtjnash December 29, 2021 16:34

KristofferC reviewed Dec 29, 2021

View reviewed changes

aviatesk commented Dec 29, 2021

View reviewed changes

aviatesk force-pushed the avi/arrayerrorcheck branch from 2ec059a to e6bec20 Compare December 30, 2021 07:31

aviatesk mentioned this pull request Dec 30, 2021

inference: add error/throwness checks for setfield! #43603

Merged

ianatol self-requested a review December 30, 2021 17:18

aviatesk force-pushed the avi/arrayerrorcheck branch from e6bec20 to 5e8bd96 Compare January 4, 2022 06:46

vtjnash reviewed Jan 7, 2022

View reviewed changes

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved

vtjnash reviewed Jan 7, 2022

View reviewed changes

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved

vtjnash reviewed Jan 7, 2022

View reviewed changes

aviatesk and others added 3 commits January 8, 2022 13:36

Apply suggestions from code review

792e27e

Co-authored-by: Jameson Nash <[email protected]>

boundcheck -> boundscheck, try to avoid splats

0fbf3df

aviatesk force-pushed the avi/arrayerrorcheck branch from 820b679 to 0fbf3df Compare January 8, 2022 04:36

aviatesk merged commit acb6e16 into master Jan 8, 2022

aviatesk deleted the avi/arrayerrorcheck branch January 8, 2022 07:26

aviatesk added a commit to aviatesk/EscapeAnalysis.jl that referenced this pull request Jan 8, 2022

reuse code from JuliaLang/julia#43587

6a0d5d8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

inference: introduce more error/throwness checks for array primitives #43587

inference: introduce more error/throwness checks for array primitives #43587

Uh oh!

aviatesk commented Dec 29, 2021 •

edited

Loading

Uh oh!

KristofferC Dec 29, 2021

Uh oh!

aviatesk Dec 29, 2021 •

edited

Loading

Uh oh!

KristofferC Dec 29, 2021

Uh oh!

aviatesk Dec 29, 2021

Uh oh!

aviatesk Dec 29, 2021 •

edited

Loading

Uh oh!

Uh oh!

vtjnash Jan 7, 2022

Uh oh!

aviatesk Jan 8, 2022

Uh oh!

Uh oh!

vtjnash left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

inference: introduce more error/throwness checks for array primitives #43587

inference: introduce more error/throwness checks for array primitives #43587

Uh oh!

Conversation

aviatesk commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KristofferC Dec 29, 2021

Choose a reason for hiding this comment

Uh oh!

aviatesk Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KristofferC Dec 29, 2021

Choose a reason for hiding this comment

Uh oh!

aviatesk Dec 29, 2021

Choose a reason for hiding this comment

Uh oh!

aviatesk Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vtjnash Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

aviatesk Jan 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vtjnash left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aviatesk commented Dec 29, 2021 •

edited

Loading

aviatesk Dec 29, 2021 •

edited

Loading

aviatesk Dec 29, 2021 •

edited

Loading