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

Fix inline assembly test #241

Closed
wants to merge 2 commits into from
Closed

Conversation

taiki-e
Copy link

@taiki-e taiki-e commented Oct 27, 2024

Depends on #240. (The first commit is from it.)

inline assembly test (tests/assembly/asm/xtensa-types.rs) is currently broken due to some issues. This PR fixes them:

  • feature(asm_experimental_arch) is required to use inline assembly.
  • "CHECK-LABEL:" for a5_i32 is wrong (typo)
  • reg tests use mov assembler macro that using or instruction. The assembler emits or, but "CHECK:" uses mov.
    • This PR fixes "CHECK:", but we might actually want to use mov.n (narrow move) instruction instead of mov.
  • freg tests are broken due to an issue will be fixed by Add XTENSA_ALLOWED_FEATURES to supported_target_features #240.

@@ -111,25 +110,25 @@ macro_rules! check_explicit_reg {

// CHECK-LABEL: a5_i8:
// CHECK: #APP
// CHECK: mov a5, a5
// CHECK: or a5, a5, a5
// CHECK: #NO_APP
check_explicit_reg!(a5_i8 i8 "a5" "mov");
Copy link
Author

Choose a reason for hiding this comment

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

Btw, this test actually appears to be doing what the ISA says it should not do.
Xtensa Instruction Set Architecture (ISA) Reference Manual (436 page) says:

ar and as should not specify the same register due to the MOV.N restriction.

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly the official ISA from cadence omits this line: https://www.cadence.com/content/dam/cadence-www/global/en_US/documents/tools/silicon-solutions/compute-ip/isa-summary.pdf (page 495). I'll ask the LLVM team what their reference is.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, it seems that what I was looking at was an old manual.

https://web.archive.org/web/20241005102231/https://0x04.net/~mwk/doc/xtensa.pdf (page 2)

Issue Date: 4/2010
RC-2010.1 Release
PD-09-0801-10-01

https://www.cadence.com/content/dam/cadence-www/global/en_US/documents/tools/silicon-solutions/compute-ip/isa-summary.pdf (page 2)

Product Release:RI-2021.8
Last Updated:04/2022
Modification: 737871

@MabezDev
Copy link
Member

Thanks for the PRs! I've rolled your changes into the current Xtensa asm commit and added you as a co-author so you still get the credit. I've started the 1.82.0.2 release, I'll ping you wen the builds are complete and you can try it out before I set it to latest.

@MabezDev MabezDev closed this Oct 28, 2024
@taiki-e taiki-e deleted the asm-test branch October 28, 2024 15:42
@MabezDev
Copy link
Member

Some of the builds are starting to finish: https://github.com/esp-rs/rust-build/releases/tag/v1.82.0.2

You can install manually with espup install --toolchain-version 1.82.0.2, or you can specify it on the Xtensa GHA like this PR did esp-rs/esp-hal#2414

Let me know if you run into any issues :)

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.

2 participants