Skip to content

Remove avx512dq and avx512vl implication for avx512fp16 #140389

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 2, 2025
Merged

Remove avx512dq and avx512vl implication for avx512fp16 #140389

merged 2 commits into from
May 2, 2025
+3 −6

Conversation

sayantn
Copy link
Contributor

@sayantn sayantn commented Apr 28, 2025 •
edited
Loading

According to Intel, avx512fp16 requires only avx512bw, but LLVM also enables avx512vl and avx512dq when avx512fp16 is active. This is relic code, and will be fixed in LLVM soon. We should remove this from Rust too asap, especially before the stabilization of AVX512

Related:

@rustbot label O-x86_64 O-x86_32 A-SIMD A-target-feature T-compiler -T-libs
r? @Amanieu

Update: the LLVM fix has been merged

cc @rust-lang/wg-llvm will it be possible to update the rustc llvm version to something after llvm/llvm-project#137450

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. A-SIMD Area: SIMD (Single Instruction Multiple Data) A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. O-x86_32 Target: x86 processors, 32 bit (like i686-*) (IA-32) O-x86_64 Target: x86-64 processors (like x86_64-*) (also known as amd64 and x64) labels Apr 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 28, 2025

⚠️ Warning ⚠️

  • Some commits in this PR modify submodules.

@rustbot rustbot removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 28, 2025
@sayantn sayantn mentioned this pull request Apr 28, 2025
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
This was referenced Apr 28, 2025
Merged
Open
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@sayantn
Copy link
Contributor Author

sayantn commented Apr 28, 2025

cc @a4lg there seems to be some doc errors in the is_riscv_feature_detected macro. Could you look into the pls.

PS: I am quite confused how they weren't caught in the stdarch CI

@sayantn
Copy link
Contributor Author

sayantn commented Apr 28, 2025 •
edited
Loading

On hindsight, this seems like a bug in rustdoc with cg_gcc. Still I am retrying the CI

cc @rust-lang/rustdoc @rust-lang/wg-gcc-backend

edit: not spurious

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@a4lg
Copy link
Contributor

a4lg commented Apr 29, 2025 •
edited
Loading

@sayantn
That's odd.

It seems the Linkcheck tool on the CI does its own job and the error itself seems valid.

However, multiple references to a single footnote is mandatory for simplicity of the new macro documentation with the platform guide.
And I thought this is allowed. While duplicate ID for footnote references is not ideal (of course) but at least tested in tests/rustdoc/footnote-reference-in-footnote-def.rs which has two duplicate IDs on footnote references to [^a].
So I suppose this is a rustdoc bug to be fixed (give each reference to a footnote unique ID as a long term solution).

If you prefer the short term solution to pass CI, you may revert rust-lang/stdarch#1779.

@a4lg a4lg mentioned this pull request Apr 29, 2025
Closed
a4lg
a4lg suggested changes Apr 29, 2025
View reviewed changes
@a4lg a4lg mentioned this pull request Apr 29, 2025
Open
@a4lg
Copy link
Contributor

a4lg commented Apr 29, 2025

I created a rustdoc PR #140434 (implementing it was easier than I thought) to enable adopting rust-lang/stdarch#1779 in the future. I'm not sure whether adoption of this PR will result in immediate success (considering the default stage for building docs (stage 0), it might not be immediate) but I hope this is accepted.

@sayantn sayantn force-pushed the avx512fp16 branch 2 times, most recently from 082ec44 to dd281cb Compare April 29, 2025 14:42
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@a4lg a4lg mentioned this pull request Apr 30, 2025
Merged
@a4lg
Copy link
Contributor

a4lg commented Apr 30, 2025

I opened a PR rust-lang/stdarch#1792 that will revert rust-lang/stdarch#1779 for now. If this PR is applied, stdarch should be safely merged into the master branch.

Even if #140434 is merged now, because the CI failure says this is caused by the stage0 compiler, we have to wait the next beta to merge rust-lang/stdarch#1779.

@sayantn
Copy link
Contributor Author

sayantn commented Apr 30, 2025

Will the recent changes to the bootstrapping process help this? I am talking about #119899

@sayantn
Copy link
Contributor Author

sayantn commented May 1, 2025

I am trying this out with rust-lang/stdarch#1792

@a4lg
Copy link
Contributor

a4lg commented May 1, 2025

I'm glad to see that this change is working! (I miss carefully designed tables and guides though)

@sayantn
Copy link
Contributor Author

sayantn commented May 1, 2025 •
edited
Loading

Nice, I will ask someone to start a try job to further make sure nothing is breaking

update: try runs are not needed, as most tests are already done. we don't need to do crater runs because this is (still) unstable

I'm glad to see that this change is working! (I miss carefully designed tables and guides though)

We can re-update stdarch with your PR after the Rustdoc fix is merged, and do another stdarch update. In the meantime, could you re-open that PR so that we don't forget about it? Thanks

@a4lg
Copy link
Contributor

a4lg commented May 1, 2025

@sayantn I filed a new issue on stdarch instead (rust-lang/stdarch#1793).
This is because merged PR cannot be reopened and re-opening closed PR requires effectively substituting everything in the PR.

@Amanieu
Copy link
Member

Amanieu commented May 1, 2025

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented May 1, 2025

📌 Commit 7443d03 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2025
VlaDexa added a commit to VlaDexa/rust that referenced this pull request May 2, 2025
@VlaDexa
Rollup merge of rust-lang#140389 - sayantn:avx512fp16, r=Amanieu
bd46521 
Remove `avx512dq` and `avx512vl` implication for `avx512fp16`

According to Intel, `avx512fp16` requires only `avx512bw`, but LLVM also enables `avx512vl` and `avx512dq` when `avx512fp16` is active. This is relic code, and will be fixed in LLVM soon. We should remove this from Rust too asap, especially before the stabilization of AVX512

Related:
 - llvm/llvm-project#136209
 - rust-lang#138940
 - rust-lang/stdarch#1781
 - rust-lang#111137

`@rustbot` label O-x86_64 O-x86_32 A-SIMD A-target-feature T-compiler -T-libs
r? `@Amanieu`

**Update: the LLVM fix has been merged**

cc `@rust-lang/wg-llvm` will it be possible to update the rustc llvm version to something after llvm/llvm-project#137450
@VlaDexa VlaDexa mentioned this pull request May 2, 2025
Closed
@Zalathar Zalathar mentioned this pull request May 2, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2025
@bors
Auto merge of rust-lang#140581 - Zalathar:rollup-ig2jb9v, r=Zalathar
7c96085 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#134034 (handle paren in macro expand for let-init-else expr)
 - rust-lang#137474 (pretty-print: Print shebang at the top of the output)
 - rust-lang#138872 (rustc_target: RISC-V `Zfinx` is incompatible with `{ILP32,LP64}[FD]` ABIs)
 - rust-lang#139046 (Improve `Lifetime::suggestion`)
 - rust-lang#139206 (std: use the address of `errno` to identify threads in `unique_thread_exit`)
 - rust-lang#139608 (Clarify `async` block behaviour)
 - rust-lang#139847 (Delegate to inner `vec::IntoIter` from `env::ArgsOs`)
 - rust-lang#140159 (Avoid redundant WTF-8 checks in `PathBuf`)
 - rust-lang#140197 (Document breaking out of a named code block)
 - rust-lang#140389 (Remove `avx512dq` and `avx512vl` implication for `avx512fp16`)
 - rust-lang#140430 (Improve test coverage of HIR pretty printing.)
 - rust-lang#140507 (rustc_target: RISC-V: feature addition batch 3)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 27d419a into rust-lang:master May 2, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone May 2, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2025
@rust-timer
Unrolled build for rust-lang#140389
208f75d 
Rollup merge of rust-lang#140389 - sayantn:avx512fp16, r=Amanieu

Remove `avx512dq` and `avx512vl` implication for `avx512fp16`

According to Intel, `avx512fp16` requires only `avx512bw`, but LLVM also enables `avx512vl` and `avx512dq` when `avx512fp16` is active. This is relic code, and will be fixed in LLVM soon. We should remove this from Rust too asap, especially before the stabilization of AVX512

Related:
 - llvm/llvm-project#136209
 - rust-lang#138940
 - rust-lang/stdarch#1781
 - rust-lang#111137

``@rustbot`` label O-x86_64 O-x86_32 A-SIMD A-target-feature T-compiler -T-libs
r? ``@Amanieu``

**Update: the LLVM fix has been merged**

cc ``@rust-lang/wg-llvm`` will it be possible to update the rustc llvm version to something after llvm/llvm-project#137450
@bors
Copy link
Collaborator

bors commented May 2, 2025

⌛ Testing commit 7443d03 with merge 7c96085...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a4lg a4lg a4lg approved these changes

Assignees

@Amanieu Amanieu

Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. O-x86_32 Target: x86 processors, 32 bit (like i686-*) (IA-32) O-x86_64 Target: x86-64 processors (like x86_64-*) (also known as amd64 and x64) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.88.0
Development

Successfully merging this pull request may close these issues.
6 participants
@sayantn @rustbot @rust-log-analyzer @a4lg @Amanieu @bors