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

Fields of global constant immutables cannot be constant-folded #18387

Closed
TotalVerb opened this issue Sep 7, 2016 · 11 comments
Closed

Fields of global constant immutables cannot be constant-folded #18387

TotalVerb opened this issue Sep 7, 2016 · 11 comments
Labels
compiler:codegen Generation of LLVM IR and native code performance Must go faster regression Regression in behavior compared to a previous version

Comments

@TotalVerb
Copy link
Contributor

TotalVerb commented Sep 7, 2016

Min-repro:

const x = 1
const y = (1,)
f() = x
g() = y[1]

Symptom:

julia> @code_llvm f()

define i64 @julia_f_71095() #0 {
top:
  ret i64 1
}

julia> @code_llvm g()

define i64 @julia_g_71096() #0 {
top:
  %0 = load i64, i64* inttoptr (i64 140144249280816 to i64*), align 16
  ret i64 %0
}

This makes z * im substantially slower than z * Complex(false, true).

If this cannot be easily fixed, it might be worth introducing @im for im. Note the performance difference:

julia> macro im(); :(Complex(false, true)); end
julia> f() = 2im
julia> g() = 2@im
julia> @code_llvm f()

define void @julia_f_71120(%Complex.68* noalias sret) #0 {
top:
  %1 = load i8, i8* inttoptr (i64 140144295071648 to i8*), align 32
  %2 = and i8 %1, 1
  %3 = zext i8 %2 to i64
  %4 = shl nuw nsw i64 %3, 1
  %5 = load i8, i8* inttoptr (i64 140144295071649 to i8*), align 1
  %6 = and i8 %5, 1
  %7 = zext i8 %6 to i64
  %8 = shl nuw nsw i64 %7, 1
  %.sroa.0.0..sroa_idx = getelementptr inbounds %Complex.68, %Complex.68* %0, i64 0, i32 0
  store i64 %4, i64* %.sroa.0.0..sroa_idx, align 8
  %.sroa.2.0..sroa_idx1 = getelementptr inbounds %Complex.68, %Complex.68* %0, i64 0, i32 1
  store i64 %8, i64* %.sroa.2.0..sroa_idx1, align 8
  ret void
}

julia> @code_llvm g()

define void @julia_g_71126(%Complex.68* noalias sret) #0 {
top:
  %.sroa.0.0..sroa_idx = getelementptr inbounds %Complex.68, %Complex.68* %0, i64 0, i32 0
  store i64 0, i64* %.sroa.0.0..sroa_idx, align 8
  %.sroa.2.0..sroa_idx1 = getelementptr inbounds %Complex.68, %Complex.68* %0, i64 0, i32 1
  store i64 2, i64* %.sroa.2.0..sroa_idx1, align 8
  ret void
}

@TotalVerb
Copy link
Contributor Author

This is a regression from v0.4:

julia> f() = 2im
f (generic function with 1 method)

julia> @code_llvm f()

define void @julia_f_21209(%Complex.12* sret) {
top:
  store %Complex.12 { i64 0, i64 2 }, %Complex.12* %0, align 8
  ret void
}

@simonster simonster added performance Must go faster regression Regression in behavior compared to a previous version compiler:codegen Generation of LLVM IR and native code labels Sep 7, 2016
@musm
Copy link
Contributor

musm commented Sep 27, 2016

See also

julia> g(x) = 2//3 * x
g (generic function with 1 method)

julia> @code_llvm g(2f0)

; Function Attrs: uwtable
define float @julia_g_71906(float) #0 {
top:
  %1 = alloca %Rational, align 8
  call void @julia_Type_71907(%Rational* noalias nonnull sret %1, %jl_value_t* inttoptr (i64 2244157328 to %jl_value_t*), i64 2, i64 3) #1
  %2 = getelementptr inbounds %Rational, %Rational* %1, i64 0, i32 0
  %3 = load i64, i64* %2, align 8
  %4 = sitofp i64 %3 to float
  %5 = getelementptr inbounds %Rational, %Rational* %1, i64 0, i32 1
  %6 = load i64, i64* %5, align 8
  %7 = sitofp i64 %6 to float
  %8 = fdiv float %4, %7
  %9 = fmul float %8, %0
  ret float %9
}

@yuyichao
Copy link
Contributor

This is a different problem....

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Sep 27, 2016

@yuyichao It's definitely part of this problem. Core.getfield won't be inlined as a constant.

@yuyichao
Copy link
Contributor

Well #18387 (comment) is a inlining problem.

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Sep 27, 2016

@yuyichao I think I have a fix for that particular inlining problem that should help with (but not resolve entirely) the problem here. Will make a WIP PR shortly.

@yuyichao
Copy link
Contributor

Sure, I'll be happily surprised if it helps with #18387 (comment) though since it doesn't have any constant immutable, is not a regression and is simply because the Rational constructor is too complicated to be inlined (it simplifies the ratio first).

@yuyichao
Copy link
Contributor

Finally found it, the rational issue is a dup of #11522

@TotalVerb
Copy link
Contributor Author

This was probably fixed by #21277.

@tkelman
Copy link
Contributor

tkelman commented Apr 16, 2017

any way to add a regression test for it?

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Apr 16, 2017

A benchmark might make more sense than a regression test, since this is only a matter of performance, not correctness.

But this would work:

0.5

julia> test18387() = im.re ? 0 : 0.0
test18387 (generic function with 1 method)

julia> @inferred(test18387())
ERROR: return type Float64 does not match inferred return type Union{Float64,Int64}
 in error(::String) at ./error.jl:21

latest beta

julia> test18387() = im.re ? 0 : 0.0
test18387 (generic function with 1 method)

julia> @inferred(test18387())
0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code performance Must go faster regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

5 participants