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

closure doesn't seem to inherit the target attributes for codegen purposes #108338

Closed
sarah-quinones opened this issue Feb 22, 2023 · 2 comments · Fixed by #116078
Closed

closure doesn't seem to inherit the target attributes for codegen purposes #108338

sarah-quinones opened this issue Feb 22, 2023 · 2 comments · Fixed by #116078
Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) 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

Comments

@sarah-quinones
Copy link

sarah-quinones commented Feb 22, 2023

I tried this code:

#![feature(avx512_target_feature)]
#![feature(stdsimd)]

#[target_feature(enable = "lzcnt,avx512f")]
pub unsafe fn test(a: &mut [u64]) {
    let mut f = move || {
        std::arch::x86_64::_mm512_mask_loadu_epi64(
            std::arch::x86_64::_mm512_setzero_si512(),
            0,
            a.as_mut_ptr() as *mut i64,
        );
    };
    f()
}

I expected to see this happen: all the avx512f functions get inlined into test

Instead, this happened: _mm512_mask_loadu_epi64 did not get inlined into test

codegen: https://godbolt.org/z/PbvexEWd8

here's a repro that mostly doesn't depend on std

#![feature(avx512_target_feature)]
#![feature(stdsimd)]
#![feature(repr_simd)]

#[repr(simd)]
#[allow(non_camel_case_types)]
#[derive(Copy, Clone)]
pub struct __m512i(i64, i64, i64, i64, i64, i64, i64, i64);

#[inline]
#[target_feature(enable = "avx512f")]
pub unsafe fn asm_fn(_: __m512i) -> __m512i {
    let mut dst = __m512i(0, 0, 0, 0, 0, 0, 0, 0);
    core::arch::asm!(
        "/* {dst} */",
        dst = inout(zmm_reg) dst,
    );
    dst
}

#[inline]
#[target_feature(enable = "avx512f")]
unsafe fn do_nothing() {}

#[target_feature(enable = "lzcnt,avx512f")]
pub unsafe fn test() {
    ({
        #[inline(always)]
        move || {
            do_nothing();
            asm_fn(__m512i(0, 0, 0, 0, 0, 0, 0, 0));
        }
    })();
}

codegen: https://godbolt.org/z/joG9xxq4c

there seem to be quite a few moving parts here.

  • if i remove the lzcnt feature (or replace it with a feature that is implied by avx512f),
  • or if i remove the inout parameter in the asm in asm_fn,
  • or if i remove the call to do_nothing,
  • or if i take the code outside the closure,

the function gets inlined as expected, so i'm not sure which is the culprit here

Meta

rustc --version --verbose:

rustc 1.69.0-nightly (5243ea5c2 2023-02-20)
binary: rustc
commit-hash: 5243ea5c29b136137c36bd773e5baa663790e097
commit-date: 2023-02-20
host: x86_64-unknown-linux-gnu
release: 1.69.0-nightly
LLVM version: 15.0.7
@sarah-quinones sarah-quinones added the C-bug Category: This is a bug. label Feb 22, 2023
@programmerjake
Copy link
Member

the closure doesn't have any target features enabled, which isn't supposed to happen according to #73631.

llvm ir before any llvm passes: https://godbolt.org/z/36fYKWbr8

link to original zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/257879-project-portable-simd/topic/weird.20codegen.20with.20_mm512_mask_loadu_epi64.20and.20lzcnt/near/329359963

@workingjubilee workingjubilee added F-target_feature_11 target feature 1.1 RFC A-SIMD Area: SIMD (Single Instruction Multiple Data) A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. labels Mar 3, 2023
@eduardosm
Copy link
Contributor

This works correctly if you add #![feature(target_feature_11)]. It looks like it was done on purpose.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Sep 23, 2023
…eature, r=Mark-Simulacrum

Add assembly test to make sure that inlining works as expected when closures inherit target features

Closes rust-lang#108338 (the added test proves that it is working correctly)
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 24, 2023
…eature, r=Mark-Simulacrum

Add assembly test to make sure that inlining works as expected when closures inherit target features

Closes rust-lang#108338 (the added test proves that it is working correctly)
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 24, 2023
…ture, r=Mark-Simulacrum

Add assembly test to make sure that inlining works as expected when closures inherit target features

Closes rust-lang#108338 (the added test proves that it is working correctly)
@bors bors closed this as completed in 67ad3c2 Sep 25, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 27, 2023
…ark-Simulacrum

Add assembly test to make sure that inlining works as expected when closures inherit target features

Closes rust-lang/rust#108338 (the added test proves that it is working correctly)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants