Skip to content

Fix uses of invalid LLVM intrinsics #1807

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sayantn
Copy link
Contributor

@sayantn sayantn commented May 27, 2025

see rust-lang/rust#140763 (comment) for context

This PR also starts checking for IR validity in tests, it is almost free, but adds a lot of confidence as we are dealing with LLVM intrinsics

sayantn added 4 commits May 28, 2025 02:04
 - The 2nd argument of the LLVM intrinsic should be IMMARG
 - use correct intrinsic for unpackl
 - fix invalid use of `simd_{or,and,xor}` on floating point vectors
 - `vec_search_string` should require `vector-enhancements-2`
 - add `-Zverify-llvm-ir` in testsuite
@rustbot
Copy link
Collaborator

rustbot commented May 27, 2025

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@folkertdev
Copy link
Contributor

Yes those functions belong to vector-enhancements-2. The test does already use that target feature, but I must have missed it at the definition.

@@ -21420,7 +21420,7 @@ pub fn vrbit_s8(a: int8x8_t) -> int8x8_t {
unsafe extern "unadjusted" {
#[cfg_attr(
any(target_arch = "aarch64", target_arch = "arm64ec"),
link_name = "llvm.aarch64.neon.rbit.v8i8"
link_name = "llvm.bitreverse.v8i8"
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: change these in cg_clif once this PR merges and rust-lang/rust updates stdarch.

@sayantn
Copy link
Contributor Author

sayantn commented May 28, 2025

Looks like llvm/llvm-project#73588 is biting us, I guess we have to use the arm-specific one

@folkertdev
Copy link
Contributor

Can you reproduce that failure in isolation? this seems to work fine https://godbolt.org/z/Mozez3P8f so I must be missing something

@folkertdev
Copy link
Contributor

folkertdev commented May 28, 2025

an https://godbolt.org/z/b1o748T5a does it, because it uses the 32-bit version. I was confused because the example from that llvm issue does work when I put it into godbolt

Comment on lines 2943 to 2947
- LLVMLink:
name: "rbit.{neon_type}"
links:
- link: "llvm.aarch64.neon.rbit.{neon_type}"
- link: "llvm.bitreverse.{neon_type}"
arch: aarch64,arm64ec
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use simd_bitreverse directly instead? That does seem to happen for e.g. simd_and

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know too much about how stdarch-gen-arm operates, particularly I don't know how I can change the impl. Still, I would try

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean model it after this:

  - name: "vand{neon_type.no}"
    doc: Vector bitwise and
    arguments: ["a: {neon_type}", "b: {neon_type}"]
    return_type: "{neon_type}"
    attr:
      - *neon-v7
      - FnCall: [cfg_attr, [*test-is-arm, {FnCall: [assert_instr, [vand]]}]]
      - FnCall: [cfg_attr, [*neon-target-aarch64-arm64ec, {FnCall: [assert_instr, [and]]}]]
      - *neon-not-arm-stable
      - *neon-cfg-arm-unstable
    safety: safe
    types:
      - int8x8_t
      - int8x16_t
      - int16x4_t
      - int16x8_t
      - int32x2_t
      - int32x4_t
      - uint8x8_t
      - uint8x16_t
      - uint16x4_t
      - uint16x8_t
      - uint32x2_t
      - uint32x4_t
      - int64x1_t
      - int64x2_t
      - uint64x1_t
      - uint64x2_t
    compose:
      - FnCall:
          - simd_and
          - - a
            - b

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

Successfully merging this pull request may close these issues.

5 participants