Aggressively propagate constant bases through ndigits and string(::Integer)

[wheeheee]

By annotating ndigits0zpb, ndigits0z, and string(::Integer) with @constprop :aggressive.

This helps ndigits(x; base) with the common constant bases of 10 or a small power of 2, removing two extra function calls (ndigits0z, ndigits0zpb) when optimized.

Also does the same with string(::Integer; base) for constant bases 2, 8, 16, and 10 (the default), so the function would then be inlined to just a call to bin, oct, hex, or dec.

On 1.13-rc1:

julia> f1(n) = ndigits(n; base=2)
f1 (generic function with 1 method)

julia> @code_typed optimize=true f1(8)
CodeInfo(
1 ─ %1 =    invoke Base.ndigits0z(n::Int64, 2::Int64)::Int64
│   %2 = intrinsic Base.slt_int(%1, 1)::Bool
│   %3 =   builtin (Core.ifelse)(%2, 1, %1)::Int64
└──      return %3
) => Int64

julia> @code_typed optimize=true string(5)
CodeInfo(
1 ─ %1 =    invoke Base.:(var"#string#391")(10::Int64, 1::Int64, #self#::typeof(string), n::Int64)::String
└──      return %1
) => String

PR:

julia> @code_typed optimize=true f1(8)
CodeInfo(
1 ─ %1  =   builtin (n === 0)::Bool
└──       goto #3 if not %1
2 ─       goto #4
3 ─ %4  = intrinsic Base.flipsign_int(n, n)::Int64
│   %5  = intrinsic Base.bitcast(UInt64, %4)::UInt64
│   %6  = intrinsic Base.ctlz_int(%5)::UInt64
│   %7  = intrinsic Base.bitcast(Int64, %6)::Int64
│   %8  = intrinsic Base.sub_int(64, %7)::Int64
└──       goto #4
4 ┄ %10 = φ (#2 => 0, #3 => %8)::Int64
│   %11 = intrinsic Base.slt_int(%10, 1)::Bool
│   %12 =   builtin (Core.ifelse)(%11, 1, %10)::Int64
└──       goto #5
5 ─       return %12
) => Int64

julia> @code_typed optimize=true string(5)
CodeInfo(
1 ─ %1 = intrinsic Base.flipsign_int(n, n)::Int64
│   %2 = intrinsic Base.bitcast(UInt64, %1)::UInt64
│   %3 = intrinsic Base.slt_int(n, 0)::Bool
│   %4 =    invoke Main.dec(%2::UInt64, 1::Int64, %3::Bool)::String
└──      return %4
) => String

[wheeheee]

Ok, the first test failure seems unrelated to the PR, but now all 4 JuliaC tests are failing after the string(::Integer) constprop commit, with

Error #1: unresolved call from statement print(
  (…)::IOBuffer,
  :(φ (%326 => %322, %348 => %343))::Any
)::Any
Stacktrace:
 [1] print_to_string(::String, ::Vararg{Any})
   @ Base strings/io.jl:143
...
 [9] __init__()
   @ libblastrampoline_jll ./libblastrampoline_jll/src/libblastrampoline_jll.jl:58
Trim verify finished with 1 error, 0 warnings.
Library flows (trim): Error During Test at /root/.julia/packages/JuliaC/bMl7u/test/programatic.jl:1
  Got exception outside of a @test
  Failed to compile /root/.julia/packages/JuliaC/bMl7u/test/lib_project/src/libtest.jl
  Stacktrace:
    [1] error(s::String)
      @ Base error.jl:56
    [2] compile_products(recipe::ImageRecipe)
      @ JuliaC ~/.julia/packages/JuliaC/bMl7u/src/compiling.jl:219
...

It's not because of the union-splitting with _base that starts getting inlined into other functions, because the next commit removes that, so I'm stuck...

[jakobnissen]

Try rebasing on master. The failure really does look unrelated.

[jakobnissen]

I threw an agent at the problem. Here's what it came up with (paraphrased by me):

• Something (not sure what) calls libblastrampoline_jll.__init__, which is type unstable even on current master. It doesn't make sense to me that your PR should expose that path
• It's type unstable due to a bullshit reason: It calls prevind, which interpolates a string, and string interpolation calls string(xs...), which loops over the xs, and therefore is type unstable for heterogenous arguments (e.g. interpolation an integer into a string)

This is not an issue with your PR, even though it maybe is triggered by your PR.

[wheeheee]

Try rebasing on master. The failure really does look unrelated.

Have already done that, the PR is currently sitting on the latest master commit.

I threw an agent at the problem. Here's what it came up with (paraphrased by me):

• Something (not sure what) calls libblastrampoline_jll.__init__, which is type unstable even on current master. It doesn't make sense to me that your PR should expose that path

Probably not, libblastrampoline_jll should be quite commonly used because people use BLAS, and wasn't a problem for JuliaC compilation until this one commit.

• It's type unstable due to a bullshit reason: It calls prevind, which interpolates a string, and string interpolation calls string(xs...), which loops over the xs, and therefore is type unstable for heterogenous arguments (e.g. interpolation an integer into a string)

This is not an issue with your PR, even though it maybe is triggered by your PR.

I really think that a slight change in inlining due to the constant propagation in string(::Integer) causes trimming to fail. That libblastrampoline_jll gets implicated is likely because it's one of the first things loaded by the module that JuliaC compiles while running tests, and the init indirectly calls string(::String, ::Int). That basically just gets inlined and calls print_to_string(::String, ::Vararg{Any}) (which calls string(::Integer)) which is where the problem manifests.

My first thought was in agreement with the conclusion that something in print_to_string somewhere, somehow becomes type unstable. I had thought it might be that the small union in the last line could have propagated upwards, by e.g. making a union of 2 types become a union of 3 or more etc., but I've 1. rewritten the code in df5e6f3 so that this isn't done, 2. inspected the optimized code_typed and found only Union{String,Int}s in print_to_string, regardless of this PR.

Also this iteration over xs has always been the case and the tests had passed, so it must be quite complicated to untangle. Perhaps the trimming mechanism is just still very fragile, if string("some", 1) can fail trimming because of a bit of inlining.

[wheeheee]

Anyway the same error can be reproduced by building Julia with this PR and running something like (after substituting appropriate directories)

"/home/user/julia/usr/bin/julia -C native -J/home/user/julia/usr/lib/julia/sys.so -g1 --startup-file=no --history-file=no --project=/tmp/jl_FxWTAl --output-o /tmp/jl_8cmvsZ/image.o.a --output-incremental=no --strip-ir --strip-metadata --experimental --trim=safe /home/user/JuliaC.jl/src/scripts/juliac-buildscript.jl --scripts-dir /home/user/JuliaC.jl/src/scripts --source /home/user/JuliaC.jl/test/lib_project/src/libtest.jl --output-lib --compile-ccallable"

It does succeed without --trim=safe, so one would have to dig into Compiler...

EDIT: after further research, it's probably related to #60331, which was closed after some lines were added to JuliaC.

setfield!(typeof(Base.print_to_string).name, :max_args, Int32(10), :monotonic)

Unfortunately while they are still there, it doesn't seem to help now...

[wheeheee]

After further investigation, I confirmed that it's indeed possible to make some trim errors go away by manually inlining certain functions in the call chain of these errors, but it's not realistic to do that as there are too many e.g. functions with exceptions that interpolate integers into error messages. I think the inlining simply enables the optimizer to elide the error branches in these cases that call string with multiple arguments, so the problem is avoided and not really fixed.

I've removed the commit that did away with the Signed/Unsigned small union since it's not the issue. Hopefully someone sees fit to investigate the trimming issues that the constant propagation causes 🙏

FWIW, the micro-benchmarks with this PR look great vs master (1d641c7). I imagine it makes the compiled code for the functions that inline them nicer as well. It also doesn't bloat the sysimage that much, the sys.so I compiled was only 0.7 MiB larger.

julia> @btime string(5);
  9.910 ns (1 allocation: 32 bytes)  # PR
  11.046 ns (1 allocation: 32 bytes) # master

julia> @btime string("foo", 5);
  86.316 ns (4 allocations: 160 bytes)  # PR
  110.577 ns (4 allocations: 160 bytes) # master

julia> @btime string("foo", 5, 6);
  118.666 ns (5 allocations: 192 bytes) # PR
  168.307 ns (5 allocations: 192 bytes) # master

[topolarity]

Sorry for the --trim troubles folks..

The cryptic (seemingly unrelated) --trim failures are almost certainly due to cache poisoning where existing results from non---trim inference can sit around and create problems for --trim inference later, which doesn't know better than to re-use these old (bad) results (#58834)

IMO we need to fix this in the 1.14 timeframe - #61983 is an attempt, but @vtjnash wants us to merge a more proper solution that will re-work our inference cache more completely

[wheeheee]

Must be quite unlucky to hit that...

[jakobnissen]

@topolarity Thanks for the input.
Given that this trim failure blocks unrelated PRs, what's the best alternative before the trimming fix lands?

1. Remove JuliaC CI from Julia until the issue is fixed, risking a bunch of PRs land which breaks trimming and which needs to be reverted once the CI is restored
2. Blocking a bunch of unrelated PRs that trigger trimming failures

I would argue 1. is probably preferable, since in practice, choosing 2. will lead to lots of time wasted, and anyone hitting a trim error in CI will say "well, trimming CI is broken anyway so we should merge this regardless", so keeping in JuliaC CI won't really buy much. But maybe that's reckless?

By the way, we should merge this since the only failures are JuliaC ;)

[KristofferC]

@topolarity are you ok with merging this? Should we temp disable JuliaC CI? Don't want to block PRs like this for weeks while there is deliberation how to fix some a bug in a separate system.

[topolarity]

@topolarity are you ok with merging this? Should we temp disable JuliaC CI? Don't want to block PRs like this for weeks while there is deliberation how to fix some a bug in a separate system.

I think I am - I'd like to try to merge #61983 ASAP but it may not be until next week that I can get to that. I am OK with temporary regressions to fix until then

Should we temp disable JuliaC CI?

We should (can't break CI)

Appreciate the follow-up ping BTW - sorry for not responding sooner @jakobnissen you caught me on vacation

[KristofferC]

We should (can't break CI)

Ok: JuliaCI/julia-buildkite#561

[wheeheee]

The macOS build / test failures are probably not related, since it's the same for other recent PRs.

Also the intermittent i686-w64-mingw32 failures, although that's a bit more concerning because

1. It's the second time I've seen that error
2. One of the 3 errors I see in the logs is (under test/precompile.jl) "break me".

Another is "spawn and wait lots of tasks". but idk.

[topolarity]

I've merged #61983 which should alleviate some of the --trim issues - @wheeheee can you do a rebase and see if things improve?

[wheeheee]

No ball. Same error.

Aside, what is happening with the macOS build / test failures...

[topolarity]

No ball. Same error.

Looks good to me: https://buildkite.com/julialang/julia-pr/builds/267#019f4972-13c6-4b68-9f78-594b96c9328c/L959

Maybe you were looking at the old build?

Aside, what is happening with the macOS build / test failures...

CI is volatile / has some regressions right now while the infrastructure is recovering from a security issue a couple weeks ago. You can peek at #base-ci-dev on Julia Slack if you're curious for updates as it's revived, but don't worry too much about the AWS-related / error: 429 failures for now

[wheeheee]

Right, probably was looking at the old one. I'm pretty sure I did click in from GitHub though...sighs

[topolarity]

Been there haha - Glad you might be unblocked here👍

[wheeheee]

All green! Good to merge now? 🙂