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

Tell LLVM about NonZero integer types #54995

Closed
wants to merge 1 commit into from

Conversation

tromey
Copy link
Contributor

@tromey tromey commented Oct 11, 2018

Change the get method on NonZero integer types to insert an
assume, so that LLVM can optimize better.

Closes #54868

Change the `get` method on NonZero integer types to insert an
`assume`, so that LLVM can optimize better.

Closes rust-lang#54868
@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2018
@dtolnay
Copy link
Member

dtolnay commented Oct 11, 2018

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned dtolnay Oct 11, 2018
pub fn assume_nonzero(x: u64, y: NonZeroU64) -> u64 {
x / y.get()
// CHECK: icmp ne i64 %y, 0
// CHECK: @llvm.assume
Copy link
Member

@scottmcm scottmcm Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth a CHECK-NOT of some sort that there's no branch/select/panic/whatever?

Also, cc @rkruppe who might have thoughts here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the assume is just a hint and might get removed at some stage of the pipeline in the future. Checking for lack of the branch is the more correct way to test this here.

@hanna-kruppe
Copy link
Contributor

I am hesitant to add assumes in random places because they add more IR (particularly if they're in a very common, small and usually-inlined function like this) and they can, unfortunately, also block some optimizations.

@tromey
Copy link
Contributor Author

tromey commented Oct 12, 2018

they can, unfortunately, also block some optimizations.

I'd like to learn what optimization this could block.

@nagisa
Copy link
Member

nagisa commented Oct 12, 2018

@tromey anything that would be blocked by a regular non-inlined call to an empty function is going to be blocked by the assume intrinsic the same way. This is what the LLVM reference has to say about it:

Note that the optimizer might limit the transformations performed on values used by the llvm.assume intrinsic in order to preserve the instructions only used to form the intrinsic’s input argument. This might prove undesirable if the extra information provided by the llvm.assume intrinsic does not cause sufficient overall improvement in code quality. For this reason, llvm.assume should not be used to document basic mathematical invariants that the optimizer can otherwise deduce or facts that are of little use to the optimizer.

In addition to that, the call to @llvm.assume might act as a side effect for the purposes of optimisations which do not explicitly take this intrinsic into account, inhibiting optimisation within the function as a whole.

@hanna-kruppe
Copy link
Contributor

Besides passes treating llvm.assume as an unknown function (which I figure can usually be fixed easily), there's also many optimizations that are gated on a value having only one use (this is quite common, grep for hasOneUse) and these are often hard to generalize to multiple uses.

@nagisa
Copy link
Member

nagisa commented Oct 12, 2018

The most appropriate way to solve this would be to assign the !range metadata to loads from and stores to NonZero, however that would require changes to the compiler instead of the library. Since NonZero<T> is a language item, it should be fairly feasible to do, however.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Oct 12, 2018

We already have range metadata on loads (there's no range metadata on stores). Without making NonZeroU32 a lang item, too -- it's a consequence of it being laid out as scalar and inheriting the niche from NonZero<u32>. Thus we get the desired optimization when there is an actual load. The problem is when the NonZeroU32 is passed as immediate or turned into one by SROA, since then there are no loads to carry the range metadata and there is no range metadata for arguments (see #50156).

@hanna-kruppe
Copy link
Contributor

Sorry, see #50156, not #50157.

@tromey
Copy link
Contributor Author

tromey commented Oct 12, 2018

Thanks.

@tromey tromey closed this Oct 12, 2018
@tromey tromey deleted the assume-nonzero branch October 12, 2018 12:31
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 25, 2020
…-obk

Unstably allow assume intrinsic in const contexts

Not sure much about this usage because there are concerns
about [blocking  optimization][1] and [slowing down LLVM][2] when using `assme` intrinsic
in inline functions.
But since Oli suggested in rust-lang#76960 (comment),
here we are.

[1]: rust-lang#54995 (comment)
[2]: rust-lang#49572 (comment)
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 25, 2020
…-obk

Unstably allow assume intrinsic in const contexts

Not sure much about this usage because there are concerns
about [blocking  optimization][1] and [slowing down LLVM][2] when using `assme` intrinsic
in inline functions.
But since Oli suggested in rust-lang#76960 (comment),
here we are.

[1]: rust-lang#54995 (comment)
[2]: rust-lang#49572 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants