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

Make repr(packed) vectors work with SIMD intrinsics #125311

Merged
merged 3 commits into from
Jun 2, 2024

Conversation

calebzulawski
Copy link
Member

In #117116 I fixed #[repr(packed, simd)] by doing the expected thing and removing padding from the layout. This should be the last step in providing a solution to rust-lang/portable-simd#319

@rustbot
Copy link
Collaborator

rustbot commented May 20, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 May 20, 2024
ty::Uint(u) => self.type_uint_from_ty(*u),
ty::RawPtr(_, _) => self.type_ptr(),
_ => unreachable!(),
};
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a function for this, Ty -> Type?

Copy link
Member

Choose a reason for hiding this comment

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

It would have to be defined in cg_llvm instead of on Ty or TyKind, since each codegen backend would have to accept them and convert it into its local "type".

Copy link
Member Author

@calebzulawski calebzulawski May 20, 2024

Choose a reason for hiding this comment

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

Yeah, I was wondering if this exists somewhere in the builder traits so it could be defined per backend.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I looked around and I think it should only live in this file, because that's a Very Dicey thing to do in the general case, but perfectly reasonable here, and all of the uses of the from_ty family are here anyways.

Copy link
Member

@workingjubilee workingjubilee May 20, 2024

Choose a reason for hiding this comment

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

( It's possible I'm wrong about the diceyness... it seems to me that in most other cases you'd want to equate types carefully and be mindful... but I'm still ambiently unsure about whether this is the correct level of abstraction. )

@@ -6,9 +6,6 @@
#[repr(simd, packed)]
struct Simd<T, const N: usize>([T; N]);

#[repr(simd)]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a silly question, but is the plan ever to use non-packed simd? Should repr(simd) just mean what this PR does, now?

After all, the existing things like __m128 are all power-of-two length, so would still get the alignment they do today. And I, at least, find it really confusing that "packed" [u32; 8] actually has 32-byte alignment.

(repr(simd) isn't on stabilization track, and if someone does want a more-aligned non-PoT simd vector they can put it into an aligned newtype.)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think we should experiment with that separately, after this PR, so that we can still back out of this path in case we find this actually hits a shitton of LLVM errors in codegen on platforms that aren't x86-64.

I agree having a single handling would have nice qualities.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, "sure, but in a future PR" seems like a reasonable answer 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is a non-zero possibility that we end up with both types--one packed and one not. Notably, adding repr(packed) changes the (internal rustc) ABI from "vector" to "aggregate" and involves an extra load. I could see a situation where someone doesn't mind the padding and wants the (chance of) slightly better codegen opportunity.

It's of course slightly odd that it's not byte-aligned, but repr(packed(N)) does take an alignment argument, with repr(packed, simd) it's just a different default alignment.

Copy link
Member

Choose a reason for hiding this comment

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

right. as far as I am concerned what we're really doing here is altering a very-poorly-defined lang item, since repr(simd) and repr(simd, packed) are Basically Magic that people can't really use directly, and what we want to think about is how to describe something that we can then expose to people.

@scottmcm
Copy link
Member

Yay! This will let us replace the to/from array implementations with transmute_unchecked, too.

Comment on lines 483 to 488
// Unpack non-power-of-2 #[repr(packed)]
let mut loaded_args = Vec::new();
for (ty, arg) in arg_tys.iter().zip(args) {
loaded_args.push(
if ty.is_simd()
&& let OperandValue::Ref(place) = arg.val
Copy link
Member

Choose a reason for hiding this comment

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

I think "unpack" is worth some more explanation.

Coming back to this after a while, I am not immediately sure why we are operating on an OperandValue::Ref. Given we are operating on repr(packed) and also on a Ref, I can think of two possible meanings for "unpack", and we could mean both.

It seems we are generating a load a few lines down, which is somewhat what I expect, but it would be nice if I don't have to guess about what the high-level intention is here. It doesn't have to be a step-by-step, just a little more descriptive.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should be better

Comment on lines +498 to +499
let loaded =
self.load_from_place(self.type_vector(elem_ll_ty, size), place);
Copy link
Member

Choose a reason for hiding this comment

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

My other question I guess is why we need to do this instead of e.g. just letting the fact the type is Copy in all the cases we care about take over.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it really matters that it's Copy, the issue is that LLVM is expecting <n x ty> but is instead getting a (possibly underaligned) pointer

let x: FullSimd<f64, 3> =
simd_add(load(Simd::<f64, 3>([0., 1., 2.])), load(Simd::<f64, 3>([2., 2., 2.])));
assert_eq!(x.0, [2., 3., 4.]);
// non-powers-of-two have padding and lesser alignment, but the intrinsic handles it
Copy link
Member

Choose a reason for hiding this comment

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

Hm. Wait. You said you removed the padding?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that was ambiguous, I think I fixed the comment

@@ -480,8 +480,55 @@ impl<'ll, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'_, 'll, 'tcx> {
}

_ if name.as_str().starts_with("simd_") => {
// Unpack non-power-of-2 #[repr(packed)]
Copy link
Member

Choose a reason for hiding this comment

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

Where are we checking for the repr(packed) attribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

I explained it better in the comments, but repr(packed) is passed by reference

@workingjubilee
Copy link
Member

Excellent, thank you.

Okay, I think my only practical desire here is to see a small codegen test that says that poking at a #[repr(packed,simd)] f32x3([f32; 3]); correctly emits something that looks like

%val = load <3 x float> %ptr, !align 4

Because that's what we're expecting, right?
Unless we already added that at some point?
Should be a fairly straightforward

// CHECK: %{{[a-z0-9_]*}} = load <3 x float> %{{[a-z0-9_]*}}, !align 4

@calebzulawski
Copy link
Member Author

I ran some codegen locally to check, but we had issues in the last PR trying to implement basically the same codegen test, I'm not sure it's worth trying again: #117116 (comment)

@scottmcm
Copy link
Member

scottmcm commented Jun 2, 2024

You can put //@ compile-flags: -C opt-level=3 in your codegen test, so it's always optimized, or you can put //@ compile-flags: -C no-prepopulate-passes to see only exactly what rust emitted, no LLVM transformation at all.

So you ought to be able to write a codegen test for this, I think? It's just that dereferencing a &Simd<_, 3> should do the load with the proper vector type and alignment, right?

Maybe you want the no-prepopulate-passes version, here, since you don't care what LLVM does with it, just to demonstrate what cg_llvm does?

@workingjubilee
Copy link
Member

I added a codegen test.

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

...I'll tidy you.

@workingjubilee
Copy link
Member

There.
@bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2024

📌 Commit 59b4583 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 Jun 2, 2024
@bors
Copy link
Contributor

bors commented Jun 2, 2024

⌛ Testing commit 59b4583 with merge 6d494e2...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 2, 2024
…nsics, r=workingjubilee

Make repr(packed) vectors work with SIMD intrinsics

In rust-lang#117116 I fixed `#[repr(packed, simd)]` by doing the expected thing and removing padding from the layout.  This should be the last step in providing a solution to rust-lang/portable-simd#319
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 2, 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 2, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Jun 2, 2024

Ah, I see. Sometimes the f32x4 vector alignment is lower than 16.

@workingjubilee
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2024

📌 Commit 5c32f84 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 Jun 2, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 2, 2024
…kingjubilee

Rollup of 3 pull requests

Successful merges:

 - rust-lang#125311 (Make repr(packed) vectors work with SIMD intrinsics)
 - rust-lang#125849 (Migrate `run-make/emit-named-files` to `rmake.rs`)
 - rust-lang#125851 (Add some more specific checks to the MIR validator)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ca9dd62 into rust-lang:master Jun 2, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 2, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 2, 2024
Rollup merge of rust-lang#125311 - calebzulawski:repr-packed-simd-intrinsics, r=workingjubilee

Make repr(packed) vectors work with SIMD intrinsics

In rust-lang#117116 I fixed `#[repr(packed, simd)]` by doing the expected thing and removing padding from the layout.  This should be the last step in providing a solution to rust-lang/portable-simd#319
@bors
Copy link
Contributor

bors commented Jun 2, 2024

⌛ Testing commit 5c32f84 with merge 5e6c2b6...

@workingjubilee
Copy link
Member

just in case bors gets any ideas:
@bors r-

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2024
…i,workingjubilee

simd packed types: remove outdated comment, extend codegen test

It seems like rust-lang#125311 made that check in codegen unnecessary?

r? `@workingjubilee` `@calebzulawski`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2024
…i,workingjubilee

simd packed types: remove outdated comment, extend codegen test

It seems like rust-lang#125311 made that check in codegen unnecessary?

r? `@workingjubilee` `@calebzulawski`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants