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.

Sign up for GitHub

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

Emit assume(false) as store i1 true, ptr poison, align 1 #127740

Closed
wants to merge 1 commit into from
Closed

Emit assume(false) as store i1 true, ptr poison, align 1 #127740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion compiler/rustc_codegen_llvm/src/intrinsic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,20 @@ impl<'ll, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'_, 'll, 'tcx> {
}

fn assume(&mut self, val: Self::Value) {
self.call_intrinsic("llvm.assume", &[val]);
match self.const_to_opt_uint(val) {
Some(0) => {
// See https://github.com/llvm/llvm-project/blob/1347b9a3aa671d610e812579ab5e5f05870586cf/llvm/docs/Frontend/PerformanceTips.rst?plain=1#L204-L211.
self.store(
Copy link
Member

Choose a reason for hiding this comment

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

request: can you please include a comment here linking to something in LLVM about why this is worth doing? Otherwise someone like me would probably come along later and go "well, llvm.assume isn't a terminator so we can just emit that all the time instead" and try to "clean it up".

(Ideally there'd be something in https://llvm.org/docs/ with a bunch of these preferred forms that we could just link to, but no need to block this PR on that.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't look like this is documented. I'll find it or add it.

Copy link
Member

Choose a reason for hiding this comment

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

For this PR if it ends up just being to a link to a comment from nikic or something that's fine -- wouldn't be the first time we have one of those in cg_llvm :P

But if you were able to get an official place in LLVM with these kinds of things, that'd be wonderful. I'd love to have a place to reference for all the "don't emit the unsigned checked math intrinsics directly" and such advice!

Copy link
Member Author

Choose a reason for hiding this comment

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

request: can you please include a comment here linking to something in LLVM about why this is worth doing? Otherwise someone like me would probably come along later and go "well, llvm.assume isn't a terminator so we can just emit that all the time instead" and try to "clean it up".

(Ideally there'd be something in https://llvm.org/docs/ with a bunch of these preferred forms that we could just link to, but no need to block this PR on that.)

See llvm/llvm-project#98910.

Copy link
Member Author

Choose a reason for hiding this comment

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

For this PR if it ends up just being to a link to a comment from nikic or something that's fine -- wouldn't be the first time we have one of those in cg_llvm :P

But if you were able to get an official place in LLVM with these kinds of things, that'd be wonderful. I'd love to have a place to reference for all the "don't emit the unsigned checked math intrinsics directly" and such advice!

I'm guessing this is what you want? https://releases.llvm.org/18.1.0/docs/Frontend/PerformanceTips.html#id10

Avoid using arithmetic intrinsics unless you are required by your source language specification to emit a particular code sequence. The optimizer is quite good at reasoning about general control flow and arithmetic, it is not anywhere near as strong at reasoning about the various intrinsics. If profitable for code generation purposes, the optimizer will likely form the intrinsics itself late in the optimization pipeline. It is very rarely profitable to emit these directly in the language frontend. This item explicitly includes the use of the overflow intrinsics.

self.const_bool(true),
self.const_poison(self.type_ptr()),
self.tcx().data_layout.i1_align.abi,
);
}
Some(_) => {}
None => {
self.call_intrinsic("llvm.assume", &[val]);
}
}
}

fn expect(&mut self, cond: Self::Value, expected: bool) -> Self::Value {
Expand Down
14 changes: 14 additions & 0 deletions tests/codegen/assume-false.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//@ compile-flags: -Cno-prepopulate-passes -Zmir-opt-level=0 -O -Zinline-mir

#![crate_type = "lib"]

#[no_mangle]
pub fn unwrap_unchecked(x: Option<i32>) -> i32 {
// CHECK-LABEL: define{{.*}} i32 @unwrap_unchecked
// CHECK-NOT: call void @llvm.assume(i1 false)
// CHECK: store i1 true, ptr poison, align 1
Copy link
Member Author

Choose a reason for hiding this comment

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

for reference: https://rust.godbolt.org/z/813oEoxso

Copy link
Member Author

Choose a reason for hiding this comment

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

This basically only occurs when ubchecks is enabled.

// CHECK-NOT: call void @llvm.assume(i1 false)
// CHECK: ret
// CHECK }
unsafe { x.unwrap_unchecked() }
}
Loading