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

docs: perf tips: deemphasize assume in favor of UnsafeAssume.jl #2181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Nov 29, 2023

The new package is dedicated to functionality like assume. It has documentation and examples, and the functions have better effects as appropriate depending on the Julia version. For example, the new package preserves a nothrow effect of the calling code. Unreleased versions of Julia are able to eliminate try-catch constructs when nothrow is known.

The new package is dedicated to functionality like `assume`. It has
documentation and examples, and the functions have better effects as
appropriate depending on the Julia version. For example, the new
package preserves a `nothrow` effect of the calling code. Unreleased
versions of Julia are able to eliminate `try`-`catch` constructs when
`nothrow` is known.
@maleadt
Copy link
Member

maleadt commented Nov 29, 2023

Appreciate the intent, but I'm not convinced. UnsafeAssume.jl is a very new package, with IMO a rather clunky API, imposing additional constraints on user code like the requirement to inline. For such a niche performance hack, it doesn't seem worth the additional dependency. If anything, that functionality should now move to a Julia intrinsic, so that the Julia optimizer can also reason about the added information (and lower it to an LLVM assumption afterwards).

@nsajko
Copy link
Contributor Author

nsajko commented Nov 29, 2023

rather clunky API

The API is the same, or rather it's a superset of the LLVM.Interop.assume API.

imposing additional constraints on user code like the requirement to inline

This actually isn't an additional constraint, it's just that I documented it. See the explanation here: https://discourse.julialang.org/t/why-does-arrayref-throw/104283/90?u=nsajko

For such a niche performance hack, it doesn't seem worth the additional dependency.

OK, your call. It seems to me that moving the functionality into a dedicated package is a good thing.

If anything, that functionality should now move to a Julia intrinsic, so that the Julia optimizer can also reason about the added information (and lower it to an LLVM assumption afterwards).

🙏, but I'm not holding my breath.

@vchuravy
Copy link
Member

, but I'm not holding my breath.

I would encourage you to contribute. Adding an intrinsic is a relatively small change.

@maleadt
Copy link
Member

maleadt commented Nov 29, 2023

The API is the same, or rather it's a superset of the LLVM.Interop.assume API.

Putting it in a separate package would be a good opportunity to improve the interface though. In the case of LLVM.jl, we don't aim to offer anything but a shallow layer over LLVM, so the LLVM.Interop.assume API is fine there. (And just to bikeshed the name, unsafe_assume_condition doesn't particularly roll off the tongue IMO.)

This actually isn't an additional constraint, it's just that I documented it. See the explanation here: https://discourse.julialang.org/t/why-does-arrayref-throw/104283/90?u=nsajko

I don't see how that applies to LLVM.Interop.assume, which is simply the LLVM intrinsic put in a definition-site inlined function. Mind explaining why that would need additional callsite inlining?

@nsajko
Copy link
Contributor Author

nsajko commented Nov 29, 2023

I've been thinking about the "make this an intrinsic" proposal, and actually I realized that's not necessary. All that's necessary is for Julia to eliminate known-unreachable (::Union{}) blocks at an early phase, so JuliaLang/julia/issues/27547. It seems there's already a stalled draft PR by @vchuravy: JuliaLang/julia/pull/37882. How difficult would reviving that PR be?

Putting it in a separate package would be a good opportunity to improve the interface though.

I thought it was fine? It's the same interface that Rust and C++ have, and I can't envision any significantly different low-level functions than assume(::Bool)::Nothing and unreachable()::Union{}.

unsafe_assume_condition doesn't particularly roll off the tongue IMO

That was kind of the point. I resent the fact that functionality like @inbounds in Base doesn't have an unsafe_ prefix, even though it breaks the usual assumptions of safety. And in general I like to give aliases to imported names, like const short = SomeModule.some_very_verbose_name, so I'm not bothered by the long name.

I don't see how that applies to LLVM.Interop.assume, which is simply the LLVM intrinsic put in a definition-site inlined function. Mind explaining why that would need additional callsite inlining?

I haven't looked into this deeply, so I'm just guessing TBH, but I think it may not be enough for just assume to be inlined into its caller. If the code that establishes the assumptions, and the code that depends on the assumptions, are not all inlined into the same function, together with assume, I don't think that Julia or LLVM will be able to make the necessary inference.

@maleadt
Copy link
Member

maleadt commented Nov 29, 2023

I've been thinking about the "make this an intrinsic" proposal, and actually I realized that's not necessary. All that's necessary is for Julia to eliminate known-unreachable (::Union{}) blocks at an early phase

How would you emit a Union{} without doing error or throw, i.e., without generating superfluous code? A Julia-level unreachable intrinsic is what I had in mind, but I haven't put much thought in this.

@nsajko
Copy link
Contributor Author

nsajko commented Nov 29, 2023

How would you emit a Union{} without doing error or throw, i.e., without generating superfluous code? A Julia-level unreachable intrinsic is what I had in mind, but I haven't put much thought in this.

My unsafe_assume_this_call_is_unreachable function already does that, and it's easy, just give Union{} as the return type to llvmcall.

So Core.Intrinsics.llvmcall("unreachable", Union{}, Tuple{}).

@nsajko
Copy link
Contributor Author

nsajko commented Nov 29, 2023

I've been thinking about the "make this an intrinsic" proposal, and actually I realized that's not necessary.

Not so sure anymore. The problem is that the llvmcall must not be eliminated (at least not too soon), so it mustn't be removable, but, ideally, one also wouldn't want the llvmcall to influence the effect inference of the code that calls it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants