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

Test codegen for repr(packed,simd) -> repr(simd) #125904

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

workingjubilee
Copy link
Member

This adds the codegen test originally requested in #117116 but exploiting the collection of features in FileCheck and compiletest to make it more resilient to expectations being broken by optimization levels. Mostly by presetting optimization levels for each revision of the tests.

I do not think the dereferenceable attribute's presence or absence is that important.

r? @calebzulawski

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 2, 2024
@workingjubilee workingjubilee force-pushed the test-packed-simd-more branch from 0692047 to 485add7 Compare June 2, 2024 23:27
@calebzulawski
Copy link
Member

thank you for taking care of all the codegen
@bors r+

@bors
Copy link
Contributor

bors commented Jun 3, 2024

📌 Commit 485add7 has been approved by calebzulawski

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 Jun 3, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 3, 2024
…, r=calebzulawski

Test codegen for `repr(packed,simd)` -> `repr(simd)`

This adds the codegen test originally requested in rust-lang#117116 but exploiting the collection of features in FileCheck and compiletest to make it more resilient to expectations being broken by optimization levels. Mostly by presetting optimization levels for each revision of the tests.

I do not think the dereferenceable attribute's presence or absence is that important.

r? `@calebzulawski`
@bors
Copy link
Contributor

bors commented Jun 3, 2024

⌛ Testing commit 485add7 with merge f760055...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 3, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 3, 2024
@workingjubilee
Copy link
Member Author

fascinating.

@workingjubilee workingjubilee force-pushed the test-packed-simd-more branch 2 times, most recently from 485fa43 to 9987363 Compare June 3, 2024 03:14
@workingjubilee
Copy link
Member Author

Slight tweak to manage the nopt builder having an even-more-unexpected test configuration.

@workingjubilee
Copy link
Member Author

@bors r=calebzulawski

@bors
Copy link
Contributor

bors commented Jun 3, 2024

📌 Commit 9987363 has been approved by calebzulawski

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 Jun 3, 2024
@bors
Copy link
Contributor

bors commented Jun 3, 2024

⌛ Testing commit 9987363 with merge 621e957...

@bors
Copy link
Contributor

bors commented Jun 3, 2024

☀️ Test successful - checks-actions
Approved by: calebzulawski
Pushing 621e957 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2024
@bors bors merged commit 621e957 into rust-lang:master Jun 3, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 3, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (621e957): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

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)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.0% [-1.0%, -1.0%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 3.4%, secondary 3.1%)

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.1%, 3.7%] 2
Regressions ❌
(secondary)
3.1% [3.1%, 3.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.4% [3.1%, 3.7%] 2

Cycles

Results (primary -1.0%, secondary 2.5%)

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)
2.5% [2.5%, 2.5%] 1
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.0% [-1.0%, -1.0%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 667.71s -> 670.518s (0.42%)
Artifact size: 318.93 MiB -> 318.94 MiB (0.00%)

@workingjubilee workingjubilee deleted the test-packed-simd-more branch June 3, 2024 20:26
@workingjubilee
Copy link
Member Author

Overall result: ✅ improvements - no action needed

doubt.jpg

// CHECK: [[VREG:%[a-z0-9_]+]] = fmul <3 x float>
// CHECK-NEXT: store <3 x float> [[VREG]], ptr [[RET_VREG]], [[RET_ALIGN]]
// CHECK-NEXT: ret void
unsafe { intrinsics::simd_mul(x, x) }
Copy link
Member

Choose a reason for hiding this comment

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

This calls simd_mul on FullSimd, which is not what rust-lang/portable-simd#422 does I think -- there the intrinsics are called on packed SIMD types. Conveniently that means they work without any other adjustments in Miri. ;)

Is there any need for simd_mul and other intrinsics to be used on types with padding?

Copy link
Member

Choose a reason for hiding this comment

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

There are some crates that use length-3 vectors with the repr_simd unstable feature. I've also played around with the idea of making the layout configurable in std::simd, though I haven't really decided if that's actually beneficial. So, I'd lean towards not necessary but a possibility

Copy link
Member

Choose a reason for hiding this comment

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

It's a nightly-only feature so breaking those crates is an option. I guess the question is whether there's any advantage to the types that have padding.

Is that tracked anywhere? If not, can you open an issue?

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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants