Skip to content

[AVR] Mulhi3/mulqi3 regression test #152902

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

tomtor
Copy link
Contributor

@tomtor tomtor commented Aug 10, 2025

See #152028

Copy link
Contributor

@Patryk27 Patryk27 left a comment

Choose a reason for hiding this comment

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

cc @benshi001 as well

@tomtor
Copy link
Contributor Author

tomtor commented Aug 10, 2025

@Patryk27 The regression is back?!

Edit: just that Rust nightly-2025-08-10-x86_64-unknown-linux-gnu fails again....

@Patryk27
Copy link
Contributor

Huh, weird - from what I've gathered, the issue is that Rust's LLVM fork generates a somewhat longer assembly output, right?

@tomtor
Copy link
Contributor Author

tomtor commented Aug 10, 2025

Huh, weird - from what I've gathered, the issue is that Rust's LLVM fork generates a somewhat longer assembly output, right?

Yes, starting 2025-08-07. But this morning nightly was ok again.

62735d2 introduced the fix after introducing the bug 3 days earlier (see commit messsge), so it is not a real Rust issue, it is in the llvm used by Rust nightly, which apparently is very up-to-date..

mulqi3 was no longer generated for an 8bit modulo operation, but instead mulhi3 (which is the longer assembly) because the 8bit value was wrongly extended to 16bit. I have no clue why Rust nightly is wrong again.

This PR just rebased on current llvm and it runs fine.

@tgross35
Copy link
Contributor

tgross35 commented Aug 11, 2025

62735d2 introduced the fix after introducing the bug 3 days earlier (see commit messsge), so it is not a real Rust issue, it is in the llvm used by Rust nightly, which apparently is very up-to-date..

Rustc stays pretty up to date - usually the major version bumps get merged as soon as we know LLVM's release date will be before Rust's stable release date for that cycle (after a few months of testing), and minor bumps happen immediately. But that commit and the one that fixed it are still too new to have made it to our fork https://github.com/rust-lang/llvm-project/commits/rustc/21.1-2025-08-01/.

Maybe it regressed with the llvm bump but got fixed with a more codegen change on Rust's side?

(probably not worth all that much investigation if it's fixed now)

@tomtor
Copy link
Contributor Author

tomtor commented Aug 11, 2025

@tgross35

Thanks for the feedback, but the nightly build fails since 2025-08-07 till today:

searched nightlies: from nightly-2025-08-01 to nightly-2025-08-08
regressed nightly: nightly-2025-08-07
searched commit range: rust-lang/rust@ec7c026...7d82b83
regressed commit: rust-lang/rust@dc0bae1

@tgross35
Copy link
Contributor

Ah sorry! I thought you were saying it was fixed in rustc (not just llvm)

@tomtor
Copy link
Contributor Author

tomtor commented Aug 11, 2025

Ah sorry! I thought you were saying it was fixed in rustc (not just llvm)

@tgross35 Its confusing, but I just tested again nightlies 2025-08-xx:

06 ok
07 fail
08 ok
09 ok
10 ok

So only 2025-08-07 failed, and it is indeed fixed now. Sorry for my inconsistent messaging, but yesterday it looked as if it failed again. Probably mixed up viewing release and debug builds.

Still strange that only 07 fails, but as you said, not worth the trouble of investigating if it is ok now. Thanks!

@tomtor tomtor marked this pull request as draft August 12, 2025 09:10
@tomtor
Copy link
Contributor Author

tomtor commented Aug 12, 2025

It is a regression in llvm:

#153156

This PR needs a different test (and a fix:-) )

(And it IS present in current Rust, but my test script tested binary sizes instead of the actual generated code :/ )

@tomtor tomtor changed the title [AVR] Add extra codegen test [AVR] Mulhi3/mulqi3 regression test Aug 12, 2025
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