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

Add support for ARM64EC #1550

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Add support for ARM64EC #1550

merged 1 commit into from
Mar 13, 2024

Conversation

dpaoliello
Copy link
Contributor

The Rust Compiler has recently added support for ARM64EC (rust-lang/rust#119199), so this change adds support for ARM64EC to stdarch by enabling the same code as AArch64.

@rustbot
Copy link
Collaborator

rustbot commented Mar 9, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks.

@@ -2074,7 +2074,7 @@ pub unsafe fn vget_low_p64(a: poly64x2_t) -> poly64x1_t {
#[target_feature(enable = "neon")]
#[rustc_legacy_const_generics(1)]
#[stable(feature = "neon_intrinsics", since = "1.59.0")]
#[cfg_attr(all(test, target_arch = "aarch64"), assert_instr(nop, IMM5 = 0))]
#[cfg_attr(all(test, any(target_arch = "aarch64", target_arch = "arm64ec")), assert_instr(nop, IMM5 = 0))]
Copy link
Member

Choose a reason for hiding this comment

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

Why is arm64rc an entirely new arch rather than a different ABI for the same arch? This means everyone who wants to use arm64 intrinsics or inline asm has to add an additional check for arm64ec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arm64EC isn't just a target feature or ABI of AArch64 - it ends up being a hybrid of AArch64 and x64 where developers need to choose the correct arch to follow at each point.

For intrinsics and assembly, it should reuse the AArch64 code (mostly: the assembly can end up being wildly different depending on what limitations you run into and OS interactions).

However, for interacting with the OS and other applications, then Arm64EC should act like x64. For example, in the backtrace library we use the x64 structs and functions: rust-lang/backtrace-rs#587.

If we were going to make Arm64EC pretend to be another arch, it would be better to treat Arm64EC as a target feature of x64 since using the wrong shape for external APIs is a correctness issue, whereas using the wrong intrinsics can be worked around by reimplementing the x64 intrinsics in AArch64. This is the approach that MSVC has taken, as it sets the predefined macros for x64 not AArch64 and then reimplements the intrinsics in softintrin.lib: https://techcommunity.microsoft.com/t5/windows-os-platform-blog/getting-to-know-arm64ec-defines-and-intrinsic-functions/ba-p/2957235. You can also see from the examples at the end of that article how much of a mess Arm64EC pretending to be x64 turns into. This also doesn't help with assembly: if we ban assembly then that is likely worse than Arm64EC having its own arch (since it guarantees an error, rather than the crate possibly falling back to some default "unknown arch" implementation) but to support it would require outlining the x64 assembly so that LLVM can generate the correct thunks (which feel like a lot of effort to generate suboptimal code).

Instead, for Rust, we decided to make Arm64EC its own arch so that these decisions are explicit, although I do acknowledge that this does create a bunch of work for library maintainers to support yet another arch despite it reusing code from one of the existing archs.

Copy link
Member

Choose a reason for hiding this comment

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

To me this feels like a bit of abuse of target_arch, which is supposed to indicate the CPU architecture and nothing about the environment. Shouldn't this be either an additional target_feature, or possibly a different target_env? There's some precedent for specifying the environment as a target feature with crt-static.

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 agree that target_arch isn't an elegant solution, but it's the approach that MSVC, LLVM and the PE format have all taken. If we were to make it a target_feature or target_env, then what would be the target_arch that it's based on? Using AArch64 leads to incorrect code, and x64 is bizarre, suboptimal and leads to issues with supporting assembly.

If you want precedence, consider 64 vs 32-bit architectures - why is x64 not just a target_feature of i686? Why not add target_pointer_width and avoid a whole new architecture, especially since i686 assembly works with x64? Because we know that pointer width is not just an architectural detail, but deeply changes the way that code interacts with the OS and other assemblies - it is better to force folks to deal with a new architecture than to assume that x64 code can interact with i686 code unchanged. This is very similar to how Arm64EC "feels" like it should be a "just" new feature or ABI, but that ignores all of the architectural details that have leaked into how assemblies interact with each other and the OS.

Copy link
Member

@calebzulawski calebzulawski Mar 10, 2024

Choose a reason for hiding this comment

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

I'm still a little confused what incorrect code would be generated with target_arch = "aarch64". In the relevant places, it would be cfg(any(target_arch = "x86_64", all(target_arch = "aarch64", target_feature = "ec"))) or similar. This is comparable to what would happen if we add the aarch64-unknown-newOS target--target_arch alone isn't sufficient to tell you how to interact with the operating system.

In the case of i686 vs x86-64, they are substantially different architectures that have some overlapping instructions. As far as I can tell, arm64ec is a standard aarch64 instruction set. If I'm mistaken, and there are aarch64 instructions that don't exist for arm64ec, then it would need its own target_arch. I just checked clang, as a comparison, which does set the __aarch64__ macro for arm64ec, which is comparable to target_arch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arm64EC is used for running x64 processes on an AArch64 Windows device - the code in that x64 process must be either x64 or Arm64EC. "Native" AArch64 code cannot run in that process. There are a few implications of this:

  • Any interactions with the OS must use the x64 shape of the API - if you have a look at that backtrace PR, you'll see we're using the x64 struct and functions. This is where you may have correctness issues: if you default to using the AArch64 target_arch code, you will use AArch64 structs and functions in an x64 process which may result in undefined behavior if there is any difference between AArch64 and x64.
  • Not all AArch64 assembly is valid Arm64EC code: Arm64EC uses a reduced set of registers, requires thunks if the code is callable from x64 and requires checkers for indirect calls (to see if the target is x64 or Arm64EC). If you default to AArch64 target_arch, then inline assembly may be invalid - for example, using a register that isn't allowed in Arm64EC will mean that Windows doesn't save it during a context switch.
  • target_feature is intended to be additive, that is any existing code that doesn't check for the feature should work on a processor that does or does not have the feature. This isn't true for Arm64EC on AArch64 target_arch for the reasons above.

I'm not sure where you're seeing Clang set __aarch64__ for Arm64EC, it seems to follow the same logic as MSVC where it pretends that Arm64EC is a variant of x64: https://github.com/llvm/llvm-project/blob/60dda1fc6ef82c5d7fe54000e6c0a21e7bafdeb5/clang/lib/Basic/Targets/AArch64.cpp#L346. Again, we could do this with Rust (make Arm64EC a feature of x64), but it greatly complicates inline assembly and is, in general, confusing for most developers. Since Rust doesn't have as much legacy baggage as C++, making Arm64EC its own arch is a cleaner and simpler story.

@Amanieu
Copy link
Member

Amanieu commented Mar 12, 2024

It looks like you forgot to re-run stdarch-gen to re-generate the generated.rs files.

@dpaoliello
Copy link
Contributor Author

It looks like you forgot to re-run stdarch-gen to re-generate the generated.rs files.

Sorry, my mistake, thanks!

@Amanieu Amanieu merged commit 166ef7b into rust-lang:master Mar 13, 2024
27 checks passed
@dpaoliello dpaoliello deleted the arm64ec branch April 15, 2024 23:21
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