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

arm64: Lift FCVTZS Wd,Dn,#n to LLIL #6280

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

droe
Copy link

@droe droe commented Dec 27, 2024

Lift FCVTZS Wd,Dn,#n to LLIL as an intrinsic.

The intrinsic is made up in the sense that I could not find an intrinsic corresponding to this instruction variant in Arm Neon Intrinsics Reference nor in arm_neon.h shipping with Xcode's clang. There is precedent for such made up intrinsics: The unsigned version of this, FCVTZU Wd,Dn,#n, was added as part of c3040ec.

I've added two tests to arm64test.py that pass (after disabling the tests_tbl test group that's currently failing for reasons unrelated to this PR). Also tested that it resolves the unimplemented instruction error with an in-the-wild binary that makes use of this instruction variant (not sharable).

@plafosse plafosse requested a review from galenbwill January 22, 2025 13:54
@galenbwill galenbwill added this to the Future Releases milestone Feb 12, 2025
@galenbwill galenbwill added Type: Enhancement Issue is a small enhancement to existing functionality State: Wontfix Issue will probably never be fixed Component: Architecture Issue needs changes to an architecture plugin Arch: ARM64 Issues with the AArch64 architecture plugin Impact: Low Issue is a papercut or has a good, supported workaround Effort: Low Issue should take < 1 week Lifting issues related to LLIL lifting labels Feb 12, 2025
@galenbwill
Copy link
Contributor

As you point out, the intrinsics are generated from the ARM C Language Extensions at https://github.com/ARM-software/acle/

This instruction variant, like all fixed-point instruction variants, is not included in that spec, and only appears in our generated intrinsics because it was manually added to our post-processed version of the spec that feeds our internal generation script.

I'm not going to merge this PR for two reasons:

  1. It is missing input and output type information
  2. The change would be overwritten the next time the intrinsics code is regenerated

I am however, going to leave the PR open as a reminder to add this instruction variant to the spec the next time we run the generator, but I am marking it "Won't Fix" for now.

Copy link
Contributor

@galenbwill galenbwill left a comment

Choose a reason for hiding this comment

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

See my comment in the conversation section for more information on why this isn't getting merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: ARM64 Issues with the AArch64 architecture plugin Component: Architecture Issue needs changes to an architecture plugin Effort: Low Issue should take < 1 week Impact: Low Issue is a papercut or has a good, supported workaround Lifting issues related to LLIL lifting State: Wontfix Issue will probably never be fixed Type: Enhancement Issue is a small enhancement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants