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

target_feature_11 rejects code that was previously accepted #108655

Closed
Tracked by #69098
tmiasko opened this issue Mar 2, 2023 · 6 comments · Fixed by #111836
Closed
Tracked by #69098

target_feature_11 rejects code that was previously accepted #108655

tmiasko opened this issue Mar 2, 2023 · 6 comments · Fixed by #111836
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-bug Category: This is a bug. F-target_feature_11 target feature 1.1 RFC P-low Low priority requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tmiasko
Copy link
Contributor

tmiasko commented Mar 2, 2023

#[target_feature(enable = "sse2")]
pub unsafe fn test() {
    ({
        #[inline(always)]
        move || {}
    })();
}

The preceding code used to compile successfully. Since the stabilization of
#![feature(target_feature_11)] in #99767 the compilation fails with:

error: cannot use `#[inline(always)]` with `#[target_feature]`
@tmiasko tmiasko added C-bug Category: This is a bug. F-target_feature_11 target feature 1.1 RFC regression-untriaged Untriaged performance or correctness regression. labels Mar 2, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 2, 2023
@apiraino
Copy link
Contributor

apiraino commented Mar 2, 2023

Hm, I think related to #108645 and #108646

@LeSeulArtichaut
Copy link
Contributor

Based on #73631 (comment), I'm guessing it would be fine to allow #[inline(always)] with #[target_feature] on closures, at least from a language perspective, even if it still trips up LLVM?

@workingjubilee
Copy link
Contributor

workingjubilee commented Mar 3, 2023

Technically, inline(always) is "just a hint" and thus one solution is and always was to accept the inline annotation and then simply ignore the request.

@workingjubilee
Copy link
Contributor

workingjubilee commented Mar 3, 2023

But yes, I believe this code actually should be fully acceptable. Note that in #108338 that if target_feature_11 is enabled, the attributes do get applied to the closure and inlining is done normally by LLVM due to a full match. No hint required.

@workingjubilee workingjubilee added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. P-low Low priority and removed regression-untriaged Untriaged performance or correctness regression. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 3, 2023
@workingjubilee
Copy link
Contributor

Assigning P-low because the regression has been contained to nightly versions due to an immediate revert, so this only blocks forward feature development.

@LeSeulArtichaut

This comment was marked as resolved.

@rustbot rustbot added requires-nightly This issue requires a nightly compiler in some way. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Mar 3, 2023
@workingjubilee workingjubilee added the A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. label Mar 3, 2023
@Nilstrieb Nilstrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@bors bors closed this as completed in 1c44af9 Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-bug Category: This is a bug. F-target_feature_11 target feature 1.1 RFC P-low Low priority requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants