effects: separate :noub effect bit from :consistent#50808
effects: separate :noub effect bit from :consistent#50808oscardssmith merged 1 commit intomasterfrom
:noub effect bit from :consistent#50808Conversation
| # function that uses `@inbounds`. However, if this `:boundscheck` is itself within an | ||
| # `@inbounds` region, its value depends on `--check-bounds`, so we need to taint | ||
| # `:consistent`-cy here also. | ||
| # `:noub`-cy here also. |
There was a problem hiding this comment.
Just took another look at this comment, and it got me wondering. Why are we checking if the current statement is inside the @inbounds region and then tainting it with :noub? Isn't necessary now as we separate caches based on --check-bounds?
Also, if we do need to taint it, probably we should taint :consistent instead?
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
test/compiler/effects.jl
Outdated
| return r | ||
| end | ||
| @test !Core.Compiler.is_consistent(Base.infer_effects(A1_inbounds)) | ||
| @test !Core.Compiler.is_noub(Base.infer_effects(A1_inbounds)) |
There was a problem hiding this comment.
No, this should taint consistent. :boundscheck is still inconsistent, but getfield with boundscheck off (or unknown) taints ub.
There was a problem hiding this comment.
Well, could you tell me why we even need to taint :consistent and/or :noub at all? As far as I understand, it would be fine if only :noinbounds is tainted, as the @inbounds does not have any effect on the @boundscheck (, which syntactically appears within the @inbounds block).
There was a problem hiding this comment.
It still has an effect of check-bounds=yes is used, so in the current design :bounds check always needs to taint consistent. Of course with the path dependency I'm adding in the other PR, we may recover consistency for the whole function, but locally, it's inconsistent
There was a problem hiding this comment.
It still has an effect of check-bounds=yes is used, so in the current design :bounds check always needs to taint consistent.
Could you please elaborate this please? When --check-bounds=[yes|no] is specified, aren't :boundscheck regarded as consistent (and thus can be inferred to return Const([true|false]))? I guess your concern is that sysimage is not separated by the flag?
There was a problem hiding this comment.
Yes, that's right. In the current design there's no guarantee that the inference check-bounds matches the runtime check-bounds, so we can't use it for IPO. We may change things around there in the near future, but for the time being that needs to be the semantics.
There was a problem hiding this comment.
Shouldn't we taint :consistent whenever we see :boundscheck except the special cased getfield call then?
|
Can we call this |
|
|
|
what about |
|
Or |
|
I don't like |
|
It would be somewhat more specific in that it is a property of effect.safe, not just a bare keyword |
19a82ef to
7938f7e
Compare
ef8fb69 to
36fc4d9
Compare
|
we have agreement between Jeff and Jameson on the noub name. |
The current `:consistent` effect bit carries dual meanings: 1. "the return value is always consistent" 2. "this method does not cause any undefined behavior". This design makes the effect bit unclear and hard to manage. Specifically, the current design prevents a post-inference analysis (as discussed in #50805) from safely refining `:consistent`-cy using post-optimization state IR. This is because it is impossible to tell whether the `:consistent`-cy has been tainted by the first or second meaning. To address this, this commit splits them into two distinct effect bits: `:consistent` for consistent return values and `:noub` for no undefined behavior. This commit also introduces an override mechanism for `:noub` as it is necessary for `@assume_effects` to concrete-evaluate the annotated methods. While this might sound risky and not in line with the existing designs of `:nonoverlayed` and `:noinbounds`, where their overrides are prohibited, but we already have an override mechanism in place for `:consistent`, which implicitly overrides `:noub`. Given this precedent, the override for `:noub` should probably be justified.
The current
:consistenteffect bit carries dual meanings:This design makes the effect bit unclear and hard to manage. Specifically, the current design prevents a post-inference analysis (as discussed in #50805) from safely refining "consistent"-cy using post-optimization state IR. This is because it is impossible to tell whether the
:consistent-cy has been tainted by the first or second meaning.To address this, this commit splits them into two distinct effect bits:
:consistentfor consistent return values and:noubfor no undefined behavior.This commit also introduces an override mechanism for
:noubas it is necessary for@assume_effectsto concrete-evaluate the annotated methods. While this might sound risky and not in line with the existing designs of:nonoverlayedand:noinbounds, where their overrides are prohibited, but we already have an override mechanism in place for:consistent, which implicitly overrides:noub. Given this precedent, the override for:noubshould probably be justified.@nanosoldier
runbenchmarks("inference", vs=":master")