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

Fix #[inline(always)] on closures with target feature 1.1 #111836

Merged
merged 2 commits into from
Jul 23, 2023
Merged

Fix #[inline(always)] on closures with target feature 1.1 #111836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddb16e2
Fix #[inline(always)] on closures with target feature 1.1
calebzulawski May 22, 2023
cdb9de7
Add codegen test ensuring always-inline closures don't bypass target …
calebzulawski Jul 17, 2023
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
17 changes: 16 additions & 1 deletion compiler/rustc_codegen_ssa/src/codegen_attrs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,22 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
});

// #73631: closures inherit `#[target_feature]` annotations
if tcx.features().target_feature_11 && tcx.is_closure(did.to_def_id()) {
//
// If this closure is marked `#[inline(always)]`, simply skip adding `#[target_feature]`.
//
// At this point, `unsafe` has already been checked and `#[target_feature]` only affects codegen.
// Emitting both `#[inline(always)]` and `#[target_feature]` can potentially result in an
// ICE, because LLVM errors when the function fails to be inlined due to a target feature
// mismatch.
//
// Using `#[inline(always)]` implies that this closure will most likely be inlined into
// its parent function, which effectively inherits the features anyway. Boxing this closure
// would result in this closure being compiled without the inherited target features, but this
// is probably a poor usage of `#[inline(always)]` and easily avoided by not using the attribute.
Comment on lines +505 to +507
Copy link
Member

Choose a reason for hiding this comment

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

Boxing seems like a waste, yes, but now that I am thinking about it, this seems like it could result in confusing behavior in the "escaping closure" case, when that would result, instead of the IIFE? Does that even make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Inlining with closures is unfortunately always confusing. Box<dyn FnOnce()>, for example, implements FnOnce itself:

rust/library/alloc/src/boxed.rs

Lines 2003 to 2009 in c4083fa

impl<Args: Tuple, F: FnOnce<Args> + ?Sized, A: Allocator> FnOnce<Args> for Box<F, A> {
type Output = <F as FnOnce<Args>>::Output;
extern "rust-call" fn call_once(self, args: Args) -> Self::Output {
<F as FnOnce<Args>>::call_once(*self, args)
}
}

This call_once doesn't have any inline attribute at all! Therefore, the boxed closure's call_once inlines into this call_once, and then it's up in the air after that.

if tcx.features().target_feature_11
&& tcx.is_closure(did.to_def_id())
Copy link
Member

Choose a reason for hiding this comment

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

...apparently is_closure will return true if this is a generator, also. I frankly have no idea how that should work, but dropping the features should remain safe in that case, at least...

&& codegen_fn_attrs.inline != InlineAttr::Always
{
let owner_id = tcx.parent(did.to_def_id());
if tcx.def_kind(owner_id).has_codegen_attrs() {
codegen_fn_attrs
Expand Down
33 changes: 33 additions & 0 deletions tests/codegen/target-feature-inline-closure.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// only-x86_64
// compile-flags: -Copt-level=3

#![crate_type = "lib"]
#![feature(target_feature_11)]

#[cfg(target_arch = "x86_64")]
use std::arch::x86_64::*;

// CHECK-LABEL: @with_avx
#[no_mangle]
#[cfg(target_arch = "x86_64")]
#[target_feature(enable = "avx")]
fn with_avx(x: __m256) -> __m256 {
// CHECK: fadd
let add = {
#[inline(always)]
|x, y| unsafe { _mm256_add_ps(x, y) }
};
add(x, x)
}

// CHECK-LABEL: @without_avx
#[no_mangle]
#[cfg(target_arch = "x86_64")]
unsafe fn without_avx(x: __m256) -> __m256 {
// CHECK-NOT: fadd
let add = {
#[inline(always)]
|x, y| unsafe { _mm256_add_ps(x, y) }
};
add(x, x)
}
18 changes: 18 additions & 0 deletions tests/ui/rfcs/rfc-2396-target_feature-11/issue-108655-inline-always-closure.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Tests #108655: closures in `#[target_feature]` functions can still be marked #[inline(always)]

// check-pass
// revisions: mir thir
// [thir]compile-flags: -Z thir-unsafeck
// only-x86_64

#![feature(target_feature_11)]

#[target_feature(enable = "avx")]
pub unsafe fn test() {
({
#[inline(always)]
move || {}
Comment on lines +13 to +14
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this forced me to check if you can annotate closures with target_feature(enable). (You cannot, fortunately.)

})();
}

fn main() {}