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

Removal of @pure makes constant-prop less effective. #49472

Open
sloede opened this issue Apr 24, 2023 · 6 comments
Open

Removal of @pure makes constant-prop less effective. #49472

sloede opened this issue Apr 24, 2023 · 6 comments
Labels
performance Must go faster regression Regression in behavior compared to a previous version

Comments

@sloede
Copy link
Contributor

sloede commented Apr 24, 2023

With the current commit on the backports-release-1.9 branch (29889c7), in StaticArrays.jl a type instability is triggered when running Julia with --check-bounds=no. I tried to create an MWE as follows:

Start Julia with

JULIA_DEPOT_PATH=$PWD/tmp-depot path/to/julia-1.9-29889c7a1c --project=. --check-bounds=no

for a clean depot. Then run the following code:

julia> using Pkg; Pkg.add(PackageSpec(name="StaticArrays", version=v"1.5.21"))

julia> using StaticArrays

julia> some_vec = SVector(1.0)
1-element SVector{1, Float64} with indices SOneTo(1):
 1.0

julia> foo(u) = SVector(u[1])
foo (generic function with 1 method)

julia> @code_warntype foo(some_vec)
MethodInstance for foo(::SVector{1, Float64})
  from foo(u) @ Main REPL[4]:1
Arguments
  #self#::Core.Const(foo)
  u::SVector{1, Float64}
Body::Any
1%1 = Base.getindex(u, 1)::Float64%2 = Main.SVector(%1)::Any
└──      return %2

When starting Julia without --check-bounds=no, the output of @code_warntype is as expected:

julia> @code_warntype foo(some_vec)
MethodInstance for foo(::SVector{1, Float64})
  from foo(u) @ Main REPL[4]:1
Arguments
  #self#::Core.Const(foo)
  u::SVector{1, Float64}
Body::SVector{1, Float64}
1%1 = Base.getindex(u, 1)::Float64%2 = Main.SVector(%1)::SVector{1, Float64}
└──      return %2

This causes a considerable performance regression to the point that code that heavily uses StaticArrays in hot loops becomes unusable.

cc @ranocha

@sloede
Copy link
Contributor Author

sloede commented Apr 24, 2023

Note that this behavior for --check-bounds=no can already be observed for Julia v1.9.0-RC1 (but wasn't noticed by me until now).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 24, 2023

Yes, this is expected. Do not use check-bounds=no if you want fast code, as it introduces undefined behavior into the code and forces Julia to emit worse code to compensate for that problem. For reference, see: #48245

@sloede
Copy link
Contributor Author

sloede commented Apr 24, 2023

I am aware of that discussion. However, if I understand it correctly, the conclusion was to not yet disallow --check-bounds=no until a way forward was found that does not break existing workflows (cc @vchuravy). However, if not even StaticArrays is going to work anymore, this effectively kills it for everyone needing to work with non-allocating arrays, i.e., a large chunk of the people and packages in the HPC world.

Are you saying that this issue is wontfix, and that with Julia v1.9, --check-bounds=no is no longer a viable option?

cc @omlins @luraess

@vchuravy
Copy link
Member

Note my observation in JuliaArrays/StaticArrays.jl#1155 (comment)

@vchuravy vchuravy changed the title Type instability in v1.9 triggered for StaticArrays when using --check-bounds=no Removal of @pure makes constant-prop less effective. Apr 24, 2023
@vchuravy
Copy link
Member

We have at least two execution environments that heavily rely on constant-propagation and can't use concrete-evaluation.

  1. GPU code uses a custom method table, with functions that are not concrete-evaluable on the Host.
  2. --check-bounds=no (for all it's faults) disables concrete-evaluation

The removal of @pure makes this worse since it prohibits work-arounds like JuliaArrays/StaticArrays.jl#1156

@aviatesk is constant propagation using the effect system? Trying @sloede test-case on JuliaArrays/StaticArrays.jl#1156 shows that Julia 1.10 will regress the situation again.

@aviatesk
Copy link
Sponsor Member

  • GPU code uses a custom method table, with functions that are not concrete-evaluable on the Host.

To increase the chances of concrete evaluation for an external compiler pipeline using a custom method table, we can separate :nonoverlayed into :nonoverlayed and :may_be_overlayed_but_still_eligible_for_concrete_eval. This allows us to concretely evaluate methods with expected overlayed behavior at runtime. For example:
E.g.

# this overlay isn't really concrete-eval-eligible
@overlay sin(x::Union{Float32,Float64}) = cos(x)

# this overlay might be concrete-eval-eligible, assuming that we are okay with having this error on CPU env during compile time
@overlay throw_some_error(x) = throw_on_gpu_env(x)
  • --check-bounds=no (for all it's faults) disables concrete-evaluation

We can also boost the chances of concrete evaluation for processes using --check-bounds=no by separately tracking :consistent and :consistent_regardless_of_boundschecking. However, after looking into #48245, my conclusion is that we should remove the flag since it only allows incorrect optimization while blocking correct optimizations, making any extra effort to optimize it unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

5 participants