Optimize checked_ilog and pow when base is a power of two

[Kmeakin]

Optimize checked_ilog and pow when the base is a power of two

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[workingjubilee]

does this affect the codegen in practical circumstances? I would expect even a fairly weak optimizer to perform this optimization, making us hand-coding it irrelevant and potentially even harmful because we introduce more conditional logic to chew through.

[Kmeakin]

does this affect the codegen in practical circumstances?

It replaces a loop by some bit manipulations. Whether anyone actually calls either function with a compile-time known, power of 2 base is another question. But it can't hurt, since it is guarded by is_statically_known.

I would expect even a fairly weak optimizer to perform this optimization, making us hand-coding it irrelevant

No, in either function, LLVM is not able to see that the loop is performing a log/pow and apply the identities: https://godbolt.org/z/6vMsxc9Kh

and potentially even harmful because we introduce more conditional logic to chew through.

They're guarded by is_statically_known so will be ignored if the base is not known at compile-time

[workingjubilee]

No, in either function, LLVM is not able to see that the loop is performing a log/pow and apply the identities: https://godbolt.org/z/6vMsxc9Kh

...then I'm kinda surprised! Nice catch.

[scottmcm]

does this affect the codegen in practical circumstances?

Would be good to have codegen tests to demonstrate what this is doing -- especially since that way there's a way for people to try removing the special cases later if they think that LLVM no longer needs them.

[workingjubilee]

thanks for the codegen tests!

[Kmeakin]

@rustbot ready

[Kmeakin]

@scottmcm ping?

[scottmcm]

This isn't really this PR's problem, but consider filing an LLVM bug (assuming it's also true in trunk) that this range is wider than necessary -- we're passing true for is_zero_poison https://llvm.org/docs/LangRef.html#llvm-ctlz-intrinsic so the range here should be range(i32 0, 32).

[Kmeakin]

The range attribute is too large, but it seems LLVM is still able to propagate the knowledge:

https://godbolt.org/z/dzxfGnhMf

[scottmcm]

The code here is looking good, but can you make sure there's normal runtime behaviour tests for it too? Notably, after the base conversation (that's fixed in the code) it made me think that that's not currently covered by any tests, so we should have something -- maybe just add to the # Examples that it returns None for base zero or one? (They seem like perfectly reasonable and helpful examples, in addition to giving coverage for this stuff.)

And maybe add some should_panic tests to ensure that the overflow checking is still correct for pow when overflow checks are enabled?

View changes since this review

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.