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

arm neon vld1*x tests fail on arm hw due to misaligned pointers #1217

Open
hkratz opened this issue Sep 11, 2021 · 9 comments
Open

arm neon vld1*x tests fail on arm hw due to misaligned pointers #1217

hkratz opened this issue Sep 11, 2021 · 9 comments

Comments

@hkratz
Copy link
Contributor

hkratz commented Sep 11, 2021

vld1x intrinsics are compiled to have alignment requirements, e.g. the assembly emitted for:

pub unsafe fn vld1_f32_x2(a: *const f32) -> float32x2x2_t { ... }

is

<stdarch_test_shim_vld1_f32_x2_vld1>:
      f4200a9f        vld1.32 {d0-d1}, [r0 :64]
      e12fff1e        bx      lr

and requires ato be 64-bit aligned (denoted by the :64 in the assembly). Clang does the same AFAICS.

Two problems:

  1. The alignment requirements are neither documented in Rust nor obvious and aarch64 has no such requirements.
  2. Our tests do not pass properly aligned pointers. Execution of the test suite on real arm hardware causes bus errors.

Possible solutions:

  1. Relax the alignment requirements at a slight general performance loss. Haven't checked if this can be done easily.
  2. Document and maybe assert the requirements and fix the tests.

cc @SparrowLii

@Amanieu
Copy link
Member

Amanieu commented Sep 12, 2021

The alignment should be that of f32. This is what clang does for the same intrinsic.

@hkratz
Copy link
Contributor Author

hkratz commented Sep 12, 2021

The alignment should be that of f32. This is what clang does for the same intrinsic.

In my test clang compiles the load with 64-bit alignment as well (vld1.32 {d16, d17}, [r1:64]): Godbolt

@hkratz hkratz changed the title arm neon vld1* tests fail on arm hw due to misaligned pointers arm neon vld1*x tests fail on arm hw due to misaligned pointers Sep 12, 2021
@hkratz
Copy link
Contributor Author

hkratz commented Sep 12, 2021

As we are directly calling the LLVM intrinsics here without any possibility to specify alignment solution 2) seems to be the only way.

@SparrowLii
Copy link
Member

As we are directly calling the LLVM intrinsics here without any possibility to specify alignment solution 2) seems to be the only way.

I agree. We can describe 1) in the document and try to find a better way in the future

@hkratz
Copy link
Contributor Author

hkratz commented Sep 13, 2021

I don't know if the LLVM behavior is intended. For vld1_f32_x4 they require 256-bit alignment (clang Godbolt).

GCC does not even have the vld1_*_x* intrinsics for armv7 (issue), so we can't compare.

Fixing the test generation is a bit of a pain, but having a failing CI on native arm hw is not a good idea. Qemu will supposedly check for unaligned access in a recent or future version (Stackoverflow answer), then the GH CI will fail as well.

@Nugine
Copy link
Contributor

Nugine commented Nov 19, 2022

Here is another test case: https://rust.godbolt.org/z/qjx6znnq4

@Amanieu
Copy link
Member

Amanieu commented Nov 29, 2023

Upstream issue: llvm/llvm-project#59081

Amanieu added a commit to Amanieu/stdarch that referenced this issue Nov 29, 2023
These seem to have been introduced by recent LLVM changes.

* The instruction limit for vld*/vst* has been raised. This is not a
significant issue, it is only used for testing.
* vld*/vst* instructions are generated with overly strict alignments:
rust-lang#1217
* vtbl/vtbx instrinsics are failing intrinsic-test for unknown reasons.
Amanieu added a commit that referenced this issue Nov 30, 2023
These seem to have been introduced by recent LLVM changes.

* The instruction limit for vld*/vst* has been raised. This is not a
significant issue, it is only used for testing.
* vld*/vst* instructions are generated with overly strict alignments:
#1217
* vtbl/vtbx instrinsics are failing intrinsic-test for unknown reasons.
@RalfJung
Copy link
Member

llvm/llvm-project#59081 is fixed; what is the status of this issue?

@Amanieu
Copy link
Member

Amanieu commented Dec 18, 2024

llvm/llvm-project@a7697c8 needs to be backported to our LLVM version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants