Add new intrinsic is_constant and optimize pow

[Centri3]

This was suggested in #114254. This basically lets LLVM know that it can evaluate the next few statements at compile-time, and thus can completely remove them. This allows us to select a path at compile-time, and with optimizations on, the other will be entirely removed. This will likely be a bit slower in debug mode but this should be ok.

I've done this for the integer's pow methods as was done in #114254 and it works great!

We should probably restrict the types that can be passed to it, though, llvm.is.constant can be passed any type but some ICE. (So far, I know that all ZSTs cause an ICE)

cc @rust-lang/lang; I highly doubt this shouldn't be insta-const, but I think discussion should be warranted anyway.

[rustbot]

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[oli-obk]

I highly doubt this shouldn't be insta-const, but I think discussion should be warranted anyway.

Make it insta-const and just always return false during const eval. It doesn't matter at const eval time 😅

[oli-obk]

This basically lets LLVM know that it can evaluate the next few statements at compile-time, and thus can completely remove them.

That's not really it. This let's llvm know that we have a code path it will be able to optimize better if the arg is const, than the code path taken if the arg is not const.

This is a hint, similar to how we have size_of checks in generic code to optimize zst code paths.

[Centri3]

That's not really it. This let's llvm know that we have a code path it will be able to optimize better if the arg is const, than the code path taken if the arg is not const.

Yeah that's true, I was referring to the behavior though. In practice it'll do basically that, folding the next statements to a constant then removing the branch altogether

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[rustbot]

Some changes occurred in src/tools/cargo

cc @ehuss

[Centri3]

?? How'd that happen?

[RalfJung]

I'm not sure if this is correct. I cannot test MIRI locally as there are tons of compile errors, so maybe somebody else can verify it?

./x.py build --stage 0 miri should always work. If it doesn't, something is wrong either with your PR or your setup. test should also work but you'll have to add a testcase first, of course. If that raises strange error, find build/ -name "miri*" | xargs rm -rf helps (but this can only really happen if you've tested Miri before so is unlikely to be your problem).

[RalfJung]

You still have a cargo submodule update for cargo in your branch, please remove that. Looks like it is in an isolated commit so a git rebase --interactive origin/master should let you remove that one commit.

Not sure how that ended up in your branch, my guess would be a rebase got wrong. Please never rebase unless necessary.

[RalfJung]

Suggested change

	let mut saw_false = false;
	let mut saw_false = false;

just a nit to make it visually more clear that these are all together testing one thing

EDIT: hm I tried to remove the empty line, not sure if github supports that...

[Centri3]

PR CI uses an older LLVM and has debug assertions enabled. Those could be causing differences.

I just ran it with rust.debug-assertions set to true yet no dice. I'll try to run with CI's version of LLVM and if that doesn't work, I'll open a zulip thread.

[oli-obk]

You can use // min-llvm-version: 16 to ensure the tests are only run on the builders with llvm 16.

It's ok for opts to differ between llvm versions, so if older llvm ends up returning false from is_constant in cases that later llvm knows to be true, that's ok

[bors]

☔ The latest upstream changes (presumably #114776) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@Centri3 any updates on this?

[Centri3]

No, I can add that comment and rebase but I won't have the time to debug it if it doesn't work

[bors]

☔ The latest upstream changes (presumably #117525) made this pull request unmergeable. Please resolve the merge conflicts.

[NCGThompson]

This could be useful for integer division too. While it is generally best to leave these kinds of optimizations to the compiler, the current version of LLVM produces sub-optimal division code in some cases. When the integer width is two machine words (u128 on 64-bit machines), LLVM only does the divide by constant optimization if the odd factor of the divisor can exactly divide 2^bits - 1. Otherwise, it calls a function. When the integer width is four machine words (u128 on 32-bit machines), LLVM divides using subtraction, even when the divisor is a constant.

[RalfJung]

Are there LLVM issues tracking these missed optimizations?

[NCGThompson]

Are there LLVM issues tracking these missed optimizations?

There are a few related issues. While these are all open as of writing, some seem to be partly out of date:

• Failure to optimize 128-bit division llvm/llvm-project#51510
• Division of uin64_t by constant on ARM32 should multiply by inverse llvm/llvm-project#37280
• optimizing division-by-constant on AArch32(ARM32) llvm/llvm-project#63731

In Rust:

• Fast algorithm for u128 (and i128) divided by small constant #54867

Many use 10 or 5 as an example. Since rustc 1.70 (corresponding to llvm-16), dividing by 10 and 5 is optimized when the dividend is 2 machine words. The issues seem to all be about dividing two machine words, while there are no issues specific to more than two words. @RalfJung, If you are thinking about posting an issue yourself, consider emphasizing 4 word division.

Here is a Compiler Explorer link to Rust code showing division by small constants. It is currently set to i686. To change the target back to the default, go to Overrides > Target architecture, and select the blank option at the top of the drop down menu. Here is a link to edit the llvm IR. Note the differences between versions 15 and 16.

I am not certain, but I have a hunch that the difference between divisors that are optimized for 2 words and divisors that aren't has to do with the divisibility of 2ⁿ - 1. I think so because of this paragraph on page 1 of T. Granlund, P. Montmery (1994):

Multiple authors [2, 11, 15] present al-
gorithms for division by constants, but only when the
divisor divides 2k − 1 for some small k. Magenheimer
et al [16, §7] give the foundation of a more general
approach, which Alverson [1] implements on the Tera
Computing System. Compiler writers are only begin-
ning to become aware of the general technique.

For reference:

• 2³² - 1 = 3 * 5 * 17 * 257 * 65537
• 2⁶⁴ - 1 = 3 * 5 * 17 * 257 * 641 * 65537 * 6700417
• 2¹²⁸ - 1 = 3 * 5 * 17 * 257 * 641 * 65537 * 274177 * 6700417 * 67280421310721

[RalfJung]

If you are thinking about posting an issue yourself, consider emphasizing 4 word division.

No I just wanted to be sure we don't carry work-arounds for LLVM issues that LLVM isn't aware of. Also now I can ping @nikic to get an estimate of whether it makes more sense to prioritize improving LLVM optimizations vs providing an is_compile_time_known intrinsic. :)

Though to be clear, nothing is blocking such an is_compile_time_known, it just needs someone to finish the work that was started in this PR.

[nikic]

@RalfJung The above discussion about divisions is irrelevant to this PR -- you obviously shouldn't use is_compile_time_known() to optimize divisions in the frontend. As for the 2.pow issue being actually solved here, using is_compile_time_known() is a very reasonable approach to that problem. I mean, we could add a loop idiom recognition pattern to LLVM to handle this, but it seems like a pretty silly thing to do, and will be fragile if what rustc emits and what LLVM matches for diverges.

[mati865]

LLVM 16 is the oldest supported version now: #117947
@Centri3 if you have the time to do the rebase it could be worth giving a try.

[NCGThompson]

Both doc tests fail when ran with ./x test core --stage 1 --doc -- is_val_statically_known. The doc tests are never ran with --stage 0 because of #[cfg(not(bootstrap())].

You need to add unsafe, use, and feature statements to make them compile. If you think they should be omitted from the docs, then you can prepend them with #.

Here is an example that passes the tests:

  /// ```rust
  /// #![feature(is_val_statically_known)]
  /// #![feature(core_intrinsics)]
  /// use std::hint::unreachable_unchecked;
  /// use std::intrinsics::is_val_statically_known;
  ///
  /// unsafe {
  ///    if !is_val_statically_known(0) { unreachable_unchecked(); }
  /// }
  /// ```
  ///
  /// This also means that the following code's behavior is unspecified; it
  /// may panic, or it may not:
  ///
  /// ```rust,no_run
  /// #![feature(is_val_statically_known)]
  /// #![feature(core_intrinsics)]
  /// use std::hint::black_box;
  /// use std::intrinsics::is_val_statically_known;
  ///
  /// unsafe {
  ///     assert_eq!(is_val_statically_known(0), black_box(is_val_statically_known(0)));
  /// }
  /// ```

[NCGThompson]

The GCC backend could implement this with __builtin_constant_p. If given a single variable as an argument, it will behave similarly to llvm.is.constant. In fact, I think I remember that llvm.is.constant was added so Clang could support __builtin_constant_p. Other backends could just interpret is_val_statically_known as false.

I don't understand the work flow for the other backends so I don't know if the intrinsic should be added in this PR or if it needs to be done in the backends separate repos.

[NCGThompson]

is incorrect to use #[cfg(debug_assertions)] here. In general, we can't assume debug_assertions matches overflow_checks even though they go together by default. This is especially true because this function has the #[rustc_inherit_overflow_checks].

Without #[cfg(debug_assertions)], it is still incomplete because the greatest possible u32 is much greater than the number of bits in an integer.

To correctly check for overflow, test to see if the final result is less than 1. If that is the case use _ = <Self>::MAX * <Self>::MAX; to force a panic iff overflow_checks are enabled.

Although it isn't necessary, you can make the code less verbose by using saturating_mul instead of overflowing_mul and by using (1 as Self).checked_shl().unwrap_or_default() instead of (1 << num_shl) * fine as Self.

[NCGThompson]

No, I can add that comment and rebase but I won't have the time to debug it if it doesn't work

@Centri3 I'd be happy to take over if you won't be able to finish it.