Skip to content

Remove the workarounds for some resolved issues #1774

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 2 commits into
base: master
Choose a base branch
from

Conversation

sayantn
Copy link
Contributor

@sayantn sayantn commented Apr 13, 2025

@rustbot
Copy link
Collaborator

rustbot commented Apr 13, 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

@sayantn sayantn closed this Apr 13, 2025
@sayantn sayantn deleted the new-amx-movrs branch April 13, 2025 23:08
@sayantn sayantn restored the new-amx-movrs branch April 13, 2025 23:09
@sayantn sayantn reopened this Apr 13, 2025
@sayantn
Copy link
Contributor Author

sayantn commented Apr 13, 2025

There seems to be some instruction check failures (specifically vdup_n and vmov_n intrinsics both are generating vmov, and before this they both generated vdup). @Amanieu do I need to fix that or is that ok to have?

@Amanieu
Copy link
Member

Amanieu commented Apr 15, 2025

There seems to be some instruction check failures (specifically vdup_n and vmov_n intrinsics both are generating vmov, and before this they both generated vdup). @Amanieu do I need to fix that or is that ok to have?

I'd just remove the tests on those (set the assert_instr to nop or just remove it). They are data movement operations and we don't guarantee a specific instruction sequence for them.

@sayantn sayantn force-pushed the new-amx-movrs branch 2 times, most recently from f4c53d7 to 5d9e007 Compare April 17, 2025 00:59
@sayantn
Copy link
Contributor Author

sayantn commented Apr 17, 2025

There are a few more changes in ASM, specifically

  • vqdmlal_high_n_s{16,32} : sqdmlal2 -> sqdmlal
  • vqdmlsl_high_n_s{16,32} : sqdmlsl2 -> sqdmlsl
  • vqdmull_high_n_s{16,32} : sqdmull2 -> sqdmull

@Amanieu are these acceptable?

@Amanieu
Copy link
Member

Amanieu commented Apr 17, 2025

That looks like a regression on the LLVM side, it should be emitting the 2 variant of the instruction.

For now this can be accepted with a FIXME, but an issue should be opened in LLVM about it.

@sayantn sayantn force-pushed the new-amx-movrs branch 3 times, most recently from 5b4c8f4 to 675eca1 Compare April 21, 2025 04:05
@sayantn
Copy link
Contributor Author

sayantn commented Apr 21, 2025

Seems like rust-lang/rust#60637 is still causing some problems in LLVM optimizing for darwin and ios, so I am delegating that

@sayantn sayantn force-pushed the new-amx-movrs branch 2 times, most recently from 8c237c1 to 62ff5cc Compare April 21, 2025 05:16
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.

3 participants