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

allow array-style simd in inline asm #110672

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

Ezrashaw
Copy link
Contributor

Required for MCP#621 to be implemented.

r? @workingjubilee

@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. labels Apr 22, 2023
@scottmcm
Copy link
Member

scottmcm commented Apr 22, 2023

Good find! And a great example of why there should be only one way to do things, so we can't miss places like this 🙂

@Ezrashaw Ezrashaw force-pushed the allow-array-simd-in-inline-asm branch from 9f5d89c to 9dbc5db Compare April 22, 2023 05:26
compiler/rustc_hir_analysis/src/check/intrinsicck.rs Outdated Show resolved Hide resolved
tests/assembly/inline-asm-avx.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/check/intrinsicck.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Member

I am aware that some of my review, if followed-through on, may wind up with code that "ends up in the same place". My reasoning is that neither you nor me are the people doing the most twiddling with our inline assembly implementation, so I think it is best if this code remains as similar to how it was as possible, even if I think the factored-out function is otherwise a good improvement and essential for this change.

@Ezrashaw Ezrashaw force-pushed the allow-array-simd-in-inline-asm branch 2 times, most recently from 709793c to 61ffb1d Compare April 22, 2023 06:19
@Ezrashaw
Copy link
Contributor Author

I am aware that some of my review, if followed-through on, may wind up with code that "ends up in the same place".

Fair enough, I've updated the PR to align with that. Also I've moved the test to the asm subdirectory.

@rust-log-analyzer

This comment has been minimized.

@Ezrashaw Ezrashaw force-pushed the allow-array-simd-in-inline-asm branch from 61ffb1d to 8534183 Compare April 22, 2023 06:34
@workingjubilee
Copy link
Member

This is a backwards-compatible change that purely adds new permitted behavior for unstable types and features. I have run the test suite and it Works On My Machine™.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 22, 2023

📌 Commit 8534183 has been approved by workingjubilee

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 Apr 22, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 22, 2023
…e-asm, r=workingjubilee

allow array-style simd in inline asm

Required for [MCP#621](rust-lang/compiler-team#621) to be implemented.

r? `@workingjubilee`
@matthiaskrgr
Copy link
Member

@bors r- rollup=iffy
failed in a rollup #110679 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 22, 2023
@Ezrashaw
Copy link
Contributor Author

I've added the force-host UI test header; I believe this should fix the problem.

@Ezrashaw Ezrashaw force-pushed the allow-array-simd-in-inline-asm branch from 8534183 to bce9b39 Compare April 22, 2023 12:56
@Noratrieb
Copy link
Member

I think we have shorter ways of making a test x86-only, don't we?

@Ezrashaw Ezrashaw force-pushed the allow-array-simd-in-inline-asm branch from bce9b39 to d31e8a4 Compare April 23, 2023 07:29
@Ezrashaw
Copy link
Contributor Author

@scottmcm I've changed the UI test headers, CI passing.

compiletest headers I will never understand lol.

@workingjubilee
Copy link
Member

I'd say "I should probably have caught that" except... yes, compiletest headers are many and confounding, sometimes.

@workingjubilee
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 24, 2023

📌 Commit d31e8a4 has been approved by workingjubilee

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2023
@workingjubilee
Copy link
Member

Opened an issue for handling this better in the test suite or tidy checks: #110751

@bors
Copy link
Contributor

bors commented Apr 24, 2023

⌛ Testing commit d31e8a4 with merge b72460f...

@bors
Copy link
Contributor

bors commented Apr 24, 2023

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing b72460f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 24, 2023
@bors bors merged commit b72460f into rust-lang:master Apr 24, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 24, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b72460f): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.1% [0.9%, 2.9%] 3
Regressions ❌
(secondary)
6.4% [5.2%, 8.1%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.1% [0.9%, 2.9%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.4% [3.4%, 3.4%] 1
Regressions ❌
(secondary)
2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.4% [3.4%, 3.4%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.4% [2.4%, 4.3%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

@rustbot rustbot added the perf-regression Performance regression. label Apr 24, 2023
@Mark-Simulacrum Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Apr 25, 2023
@Mark-Simulacrum
Copy link
Member

Cranelift-codegen and keccak are known to exhibit bimodality.

@Ezrashaw Ezrashaw deleted the allow-array-simd-in-inline-asm branch April 26, 2023 09:03
@Ezrashaw Ezrashaw restored the allow-array-simd-in-inline-asm branch May 3, 2024 08:11
@Ezrashaw Ezrashaw deleted the allow-array-simd-in-inline-asm branch May 13, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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
Development

Successfully merging this pull request may close these issues.

Documentation on vector arguments for inline assembly is inconsistent with observed behavior