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

@inbounds in dead code taints consistency #48243

Closed
Keno opened this issue Jan 11, 2023 · 8 comments · Fixed by #48246
Closed

@inbounds in dead code taints consistency #48243

Keno opened this issue Jan 11, 2023 · 8 comments · Fixed by #48246
Assignees
Labels
compiler:effects effect analysis

Comments

@Keno
Copy link
Member

Keno commented Jan 11, 2023

function foo()
      false && @inbounds (1,2,3)[1]
      return 1
end
julia> Base.infer_effects(foo)
(!c,+e,+n,+t,+s,+m)

This likely regressed in #48158

@giordano giordano added the compiler:effects effect analysis label Jan 11, 2023
@Keno
Copy link
Member Author

Keno commented Jan 11, 2023

On further thought, I'm not sure :inbounds actually needs to taint consistency at all. The thing that taints consistency is :boundscheck. If we didn't have --check-bounds=yes it would make a difference, but given that we have all the options, I'm not sure it does.

@gbaraldi
Copy link
Member

In fact :inbounds might make it more consistent, since it won't give a bounds error ever, it will either segfault or return garbage, but indexing out of bounds is undefined anyway.

@Keno
Copy link
Member Author

Keno commented Jan 11, 2023

Consistency needs to be true for all argument values, since the compiler is allowed to invent them.

@gbaraldi
Copy link
Member

But if we access outside of bounds and into potentially undefined memory, how can the compiler know what to put there?

@Keno
Copy link
Member Author

Keno commented Jan 11, 2023

The compiler doesn't, we need to prevent trying to concrete eval something that might invoke undefined behavior or in other words, undefined behavior taints :consistent.

@oscardssmith
Copy link
Member

Could we make it so that indexing out of bounds at compile time semantically returned zero?

@gbaraldi
Copy link
Member

If we knew at compile time that were indexing out of bounds we probably want to throw an error at compile time no?

@Keno
Copy link
Member Author

Keno commented Jan 11, 2023

You'd have to compile a separate copy of all functions, which is unlikely to be worth it. Also in general, you can't really do much with the results, because it's no longer a faithful model of the function being analyzed.

@Keno Keno self-assigned this Jan 12, 2023
Keno added a commit that referenced this issue Jan 12, 2023
This works to fix #48243, by only tanting effects if an `@inbounds` statement
is actually reached. Further, it refines the `noinbounds` effect to be
IPO-cached and used to track whether a particular method read the inbounds state.
A `:boundscheck` expression now does not immediately taint consistencty, but
instead, taints `noinbounds` only. Then, if a method that has `:noinbounds`
tainted is called within an `@inbounds` region, consistency is tainted.
Similarly, a tainted `:noinbounds` disables constant propagation at
`@inbounds` statements or if the method propagates inbounds.
Keno added a commit that referenced this issue Jan 12, 2023
This works to fix #48243, by only tanting effects if an `@inbounds` statement
is actually reached. Further, it refines the `noinbounds` effect to be
IPO-cached and used to track whether a particular method read the inbounds state.
A `:boundscheck` expression now does not immediately taint consistencty, but
instead, taints `noinbounds` only. Then, if a method that has `:noinbounds`
tainted is called within an `@inbounds` region, consistency is tainted.
Similarly, a tainted `:noinbounds` disables constant propagation at
`@inbounds` statements or if the method propagates inbounds.
Keno added a commit that referenced this issue Jan 12, 2023
This works to fix #48243, by only tanting effects if an `@inbounds` statement
is actually reached. Further, it refines the `noinbounds` effect to be
IPO-cached and used to track whether a particular method read the inbounds state.
A `:boundscheck` expression now does not immediately taint consistencty, but
instead, taints `noinbounds` only. Then, if a method that has `:noinbounds`
tainted is called within an `@inbounds` region, consistency is tainted.
Similarly, a tainted `:noinbounds` disables constant propagation at
`@inbounds` statements or if the method propagates inbounds.
Keno added a commit that referenced this issue Jan 12, 2023
This works to fix #48243, by only tanting effects if an `@inbounds` statement
is actually reached. Further, it refines the `noinbounds` effect to be
IPO-cached and used to track whether a particular method read the inbounds state.
A `:boundscheck` expression now does not immediately taint consistencty, but
instead, taints `noinbounds` only. Then, if a method that has `:noinbounds`
tainted is called within an `@inbounds` region, consistency is tainted.
Similarly, a tainted `:noinbounds` disables constant propagation at
`@inbounds` statements or if the method propagates inbounds.
KristofferC pushed a commit that referenced this issue Jan 16, 2023
This works to fix #48243, by only tanting effects if an `@inbounds` statement
is actually reached. Further, it refines the `noinbounds` effect to be
IPO-cached and used to track whether a particular method read the inbounds state.
A `:boundscheck` expression now does not immediately taint consistencty, but
instead, taints `noinbounds` only. Then, if a method that has `:noinbounds`
tainted is called within an `@inbounds` region, consistency is tainted.
Similarly, a tainted `:noinbounds` disables constant propagation at
`@inbounds` statements or if the method propagates inbounds.

(cherry picked from commit d544e78)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:effects effect analysis
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants