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

Documentation on vector arguments for inline assembly is inconsistent with observed behavior #106924

Closed
pirocks opened this issue Jan 16, 2023 · 2 comments · Fixed by #110672
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-inline-assembly Area: Inline assembly (`asm!(…)`) 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. PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pirocks
Copy link

pirocks commented Jan 16, 2023

I tried this code:

#![feature(portable_simd)]

use std::simd::{i64x8, f64x8};
use std::arch::asm;


pub fn convert(a: i64x8) -> f64x8{
    let converted: f64x8;
    unsafe {
        asm!(
        "vcvtqq2pd {converted} {a}",
        a = in(zmm_reg) a,
        converted = out(zmm_reg) converted,
        );
    }
    converted
}

I expected to see this happen:
I thought this would compile and do as it says since, https://doc.rust-lang.org/reference/inline-assembly.html states that zmm_reg accepts vector types., like f64x8 and i64x8.

Instead, this happened:

error: cannot use value of type `Simd<i64, 8>` for inline assembly
  --> <source>:12:25
   |
12 |         a = in(zmm_reg) a,
   |                         ^
   |
   = note: only integers, floats, SIMD vectors, pointers and function pointers can be used as arguments for inline assembly

error: cannot use value of type `Simd<f64, 8>` for inline assembly
  --> <source>:13:34
   |
13 |         converted = out(zmm_reg) converted,
   |                                  ^^^^^^^^^
   |
   = note: only integers, floats, SIMD vectors, pointers and function pointers can be used as arguments for inline assembly

error: aborting due to 2 previous errors

Meta

rustc --version --verbose:

<redacted>@<redacted>::~$ rustc --version --verbose
rustc 1.68.0-nightly (afaf3e07a 2023-01-14)
binary: rustc
commit-hash: afaf3e07aaa7ca9873bdb439caec53faffa4230c
commit-date: 2023-01-14
host: x86_64-unknown-linux-gnu
release: 1.68.0-nightly
LLVM version: 15.0.6

No backtrace applicable.

@pirocks pirocks added the C-bug Category: This is a bug. label Jan 16, 2023
@workingjubilee workingjubilee added A-SIMD Area: SIMD (Single Instruction Multiple Data) PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) A-diagnostics Area: Messages for errors, warnings, and lints labels Jan 16, 2023
@workingjubilee
Copy link
Member

workingjubilee commented Jan 16, 2023

This is partly a diagnostics issue. The type you refer to in std::simd is more abstract, in that we permit it to be compiled in a way that does not actually use an AVX512 register. The type that the asm! documentation is trying to refer to is... very not. Thus it needs a conversion step to/from the "definitely a hardware register" type. The implementation that we should be recommending is:

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

use std::arch::{asm, x86_64::{__m512d, __m512i}};
use std::simd::{f64x8, i64x8};

#[target_feature(enable = "avx512f")]
#[target_feature(enable = "avx512dq")]
pub unsafe fn convert(a: i64x8) -> f64x8 {
    let a: __m512i = a.into();
    let converted: __m512d;
    unsafe {
        asm! {
            "vcvtqq2pd {converted} {a}",
            a = in(zmm_reg) a,
            converted = out(zmm_reg) converted,
        };
    }
    converted.into()
}

Obviously that's a fair few steps to get from here to there and we can also consider ways to smooth this path further.

@workingjubilee workingjubilee added A-inline-assembly Area: Inline assembly (`asm!(…)`) A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Jan 16, 2023
@workingjubilee workingjubilee added the A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. label Mar 3, 2023
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@workingjubilee workingjubilee linked a pull request Apr 22, 2023 that will close this issue
@workingjubilee
Copy link
Member

It turns out this was actually more possible than I expected by simply changing how inline asm! accepts arguments, and validating that Simd still has an equivalence to a register, so this code now passes:

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

use std::simd::{i64x8, f64x8};
use std::arch::asm;

#[target_feature(enable = "avx512f")]
pub unsafe fn convert(a: i64x8) -> f64x8{
    let converted: f64x8;
    unsafe {
        asm!(
        "vcvtqq2pd {converted}, {a}",
        a = in(zmm_reg) a,
        converted = out(zmm_reg) converted,
        );
    }
    converted
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-inline-assembly Area: Inline assembly (`asm!(…)`) 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. PG-portable-simd Project group: Portable SIMD (https://github.com/rust-lang/project-portable-simd) 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.

3 participants