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

const vector passed through to codegen #128537

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

Jamesbarford
Copy link
Contributor

This allows constant vectors using a repr(simd) type to be propagated
through to the backend by reusing the functionality used to do a similar
thing for the simd_shuffle intrinsic

#118209

r​? RalfJung

@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 2, 2024
compiler/rustc_codegen_ssa/src/mir/constant.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/mir/constant.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/common.rs Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/mir/operand.rs Outdated Show resolved Hide resolved
tests/codegen/const-vector.rs Outdated Show resolved Hide resolved
tests/codegen/const-vector.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2024

This looks good to me apart from the comments, but I'd prefer if some LLVM person could look at it as well.
r? @nikic
Also, let's Cc some of the usual SIMD suspects just to be sure.
Cc @Amanieu @workingjubilee @calebzulawski

@rustbot rustbot assigned nikic and unassigned lcnr Aug 2, 2024
@calebzulawski
Copy link
Member

calebzulawski commented Aug 2, 2024

Can you add an instance of #[repr(simd, packed)] to a test to make sure that still works? It's not encoded as a vector type in LLVM, but a regular array.

@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2024

Ah good point, we need to either still encode that as an array or just exclude it from this new special treatment.

@calebzulawski
Copy link
Member

Looking at the existing code I have a feeling that this is already skipped in that case, but I'd like to be sure

@Jamesbarford
Copy link
Contributor Author

Jamesbarford commented Aug 2, 2024

Thanks for the feedback.

Regarding the test, I'm a little unfamiliar with how to represent a #[repr(simd, packed)] struct.

I have the following bellow and using rustc 1.81.0-nightly it gets treated in the same way as a const { type_name(2,4,6,8) }. However I'm not sure this is what you mean?

#[repr(simd, packed)]
#[derive(Copy, Clone)]
pub struct Simd<T, const N: usize>([T; N]);

extern "unadjusted" {
    #[no_mangle]
    fn test_simd(a: Simd<i32, 4>);
}

fn main() {
    // without optimisations: void @test_simd(<4 x i32> %some_variable_name)
    // with optimisations: tail call void @test_simd(<4 x i32> <i32 2, i32 4, i32 6, i32 8>)
    unsafe { test_simd(const { Simd::<i32, 4>([2, 4, 6, 8]) }) }
}

@calebzulawski
Copy link
Member

calebzulawski commented Aug 2, 2024

For a power of 2 length it should be treated the same, so that's good. If you use another length (e.g. 3) it should instead be an array, not a vector (in LLVM IR)

@Jamesbarford
Copy link
Contributor Author

Ah, yes then this PR does change things for (continuing with the above example, but making it 3 elements instead) Simd<i32, 3>. Of which I have shown below and am not sure of the implications.

Is there, forgive my ignorance, something along the lines of an is_packed() method or something similar so we can make this not happen for the packed case?

With these changes

Without optimisations:

void @test_simd(%"Simd<i32, 3>" bitcast (<3 x i32> <i32 2, i32 4, i32 6> to %"Simd<i32, 3>"))

And with:

tail call void @test_simd(%"Simd<i32, 3>" bitcast (<3 x i32> <i32 2, i32 4, i32 6> to %"Simd<i32, 3>"))

rustc 1.81.0-nightly

Without optimisations:

%9 = load %"Simd<i32, 3>", ptr %0, align 4
call void @test_simd(%"Simd<i32, 3>" %9) #3

And with:

tail call void @test_simd(%"Simd<i32, 3>" { [3 x i32] [i32 2, i32 4, i32 6] })

@RalfJung
Copy link
Member

RalfJung commented Aug 5, 2024

Is there, forgive my ignorance, something along the lines of an is_packed() method or something similar so we can make this not happen for the packed case?

I think the most reliable way is to compute the layout of the type, and ensure that it has Abi::Vector. You can even do this instead of is_simd. That will also cover newtypes and similar scenarios, and it basically matches what is already done for integers and similar scalar types.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

compiler/rustc_codegen_ssa/src/mir/constant.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/mir/operand.rs Outdated Show resolved Hide resolved
tests/codegen/const-vector.rs Outdated Show resolved Hide resolved
Comment on lines 652 to 654
_ => self.eval_mir_constant_to_operand(bx, constant),
};
}

self.eval_mir_constant_to_operand(bx, constant)
Copy link
Member

Choose a reason for hiding this comment

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

You still have two call sites for eval_mir_constant_to_operand here.

If you early-return the OperandRef above, you can just have a single call site after the if.

Comment on lines 640 to 644
// Make sure this type is actually passed as an immediate vector.
// (In particular, this excludes packed SIMD vectors.)
if constant_ty.is_simd() {
let layout = bx.layout_of(constant_ty);
return match layout.abi {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Make sure this type is actually passed as an immediate vector.
// (In particular, this excludes packed SIMD vectors.)
if constant_ty.is_simd() {
let layout = bx.layout_of(constant_ty);
return match layout.abi {
// Most SIMD vector constants should be passed as immediates.
// (In particular, some intrinsics really rely on this.)
if constant_ty.is_simd() {
// However, some SIMD types do not actually use the vector ABI
// (in particular, packed SIMD types do not). Ensure we exclude those.
let layout = bx.layout_of(constant_ty);
return match layout.abi {

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2024

Ah, the GCC backend doesn't like this becoming a struct rather than a vector... okay, let's put back the if ty_is_simd { then, and do the simd_shuffle cleanup later. Sorry for the detour.

@Jamesbarford
Copy link
Contributor Author

Ah, the GCC backend doesn't like this becoming a struct rather than a vector... okay, let's put back the if ty_is_simd { then, and do the simd_shuffle cleanup later. Sorry for the detour.

Thanks, I believe I have now addressed your outstanding points.

@RalfJung
Copy link
Member

RalfJung commented Aug 8, 2024

Yeah, looks good to me, thanks!
Please squash the commits.

@nikic
Copy link
Contributor

nikic commented Aug 12, 2024

@bors r=RalfJung,nikic

@bors
Copy link
Contributor

bors commented Aug 12, 2024

📌 Commit 27ca35a has been approved by RalfJung,nikic

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 Aug 12, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2024
…llaumeGomez

Rollup of 10 pull requests

Successful merges:

 - rust-lang#128149 (nontemporal_store: make sure that the intrinsic is truly just a hint)
 - rust-lang#128394 (Unify run button display with "copy code" button and with mdbook buttons)
 - rust-lang#128537 (const vector passed through to codegen)
 - rust-lang#128632 (std: do not overwrite style in `get_backtrace_style`)
 - rust-lang#128878 (Slightly refactor `Flags` in bootstrap)
 - rust-lang#128886 (Get rid of some `#[allow(rustc::untranslatable_diagnostic)]`)
 - rust-lang#128929 (Fix codegen-units tests that were disabled 8 years ago)
 - rust-lang#128937 (Fix warnings in rmake tests on `x86_64-unknown-linux-gnu`)
 - rust-lang#128978 (Use `assert_matches` around the compiler more)
 - rust-lang#128994 (Fix bug in `Parser::look_ahead`.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit aea5087 into rust-lang:master Aug 12, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 12, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2024
Rollup merge of rust-lang#128537 - Jamesbarford:118980-const-vector, r=RalfJung,nikic

const vector passed through to codegen

This allows constant vectors using a repr(simd) type to be propagated
through to the backend by reusing the functionality used to do a similar
thing for the simd_shuffle intrinsic

rust-lang#118209

r​? RalfJung
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 27, 2024
…rkingjubilee

simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 27, 2024
…rkingjubilee

simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2024
Rollup merge of rust-lang#128731 - RalfJung:simd-shuffle-vector, r=workingjubilee

simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 28, 2024
simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang/rust#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang/rust#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Aug 28, 2024
simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang/rust#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang/rust#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 14, 2024
…r=compiler-errors

simd_shuffle: require index argument to be a vector

Remove some codegen hacks by forcing the SIMD shuffle `index` argument to be a vector, which means (thanks to rust-lang#128537) that it will automatically be passed as an immediate in LLVM. The only special-casing we still have is for the extra sanity-checks we add that ensure that the indices are all in-bounds. (And the GCC backend needs to do a bunch of work since the Rust intrinsic is modeled after what LLVM expects, which seems to be quite different from what GCC expects.)

Fixes rust-lang#128738, see that issue for more context.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 14, 2024
…compiler-errors

simd_shuffle: require index argument to be a vector

Remove some codegen hacks by forcing the SIMD shuffle `index` argument to be a vector, which means (thanks to rust-lang#128537) that it will automatically be passed as an immediate in LLVM. The only special-casing we still have is for the extra sanity-checks we add that ensure that the indices are all in-bounds. (And the GCC backend needs to do a bunch of work since the Rust intrinsic is modeled after what LLVM expects, which seems to be quite different from what GCC expects.)

Fixes rust-lang#128738, see that issue for more context.
fmease added a commit to fmease/rust that referenced this pull request Sep 14, 2024
…r=compiler-errors

simd_shuffle: require index argument to be a vector

Remove some codegen hacks by forcing the SIMD shuffle `index` argument to be a vector, which means (thanks to rust-lang#128537) that it will automatically be passed as an immediate in LLVM. The only special-casing we still have is for the extra sanity-checks we add that ensure that the indices are all in-bounds. (And the GCC backend needs to do a bunch of work since the Rust intrinsic is modeled after what LLVM expects, which seems to be quite different from what GCC expects.)

Fixes rust-lang#128738, see that issue for more context.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 14, 2024
Rollup merge of rust-lang#130268 - RalfJung:simd-shuffle-idx-vector, r=compiler-errors

simd_shuffle: require index argument to be a vector

Remove some codegen hacks by forcing the SIMD shuffle `index` argument to be a vector, which means (thanks to rust-lang#128537) that it will automatically be passed as an immediate in LLVM. The only special-casing we still have is for the extra sanity-checks we add that ensure that the indices are all in-bounds. (And the GCC backend needs to do a bunch of work since the Rust intrinsic is modeled after what LLVM expects, which seems to be quite different from what GCC expects.)

Fixes rust-lang#128738, see that issue for more context.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 16, 2024
…r-errors

simd_shuffle: require index argument to be a vector

Remove some codegen hacks by forcing the SIMD shuffle `index` argument to be a vector, which means (thanks to rust-lang/rust#128537) that it will automatically be passed as an immediate in LLVM. The only special-casing we still have is for the extra sanity-checks we add that ensure that the indices are all in-bounds. (And the GCC backend needs to do a bunch of work since the Rust intrinsic is modeled after what LLVM expects, which seems to be quite different from what GCC expects.)

Fixes rust-lang/rust#128738, see that issue for more context.
antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this pull request Oct 9, 2024
simd_shuffle intrinsic: allow argument to be passed as vector

See rust-lang/rust#128738 for context.

I'd like to get rid of [this hack](https://github.com/rust-lang/rust/blob/6c0b89dfac65be9a5be12f938f23098ebc36c635/compiler/rustc_codegen_ssa/src/mir/block.rs#L922-L935). rust-lang/rust#128537 almost lets us do that since constant SIMD vectors will then be passed as immediate arguments. However, simd_shuffle for some reason actually takes an *array* as argument, not a vector, so the hack is still required to ensure that the array becomes an immediate (which then later stages of codegen convert into a vector, as that's what LLVM needs).

This PR prepares simd_shuffle to also support a vector as the `idx` argument. Once this lands, stdarch can hopefully be updated to pass `idx` as a vector, and then support for arrays can be removed, which finally lets us get rid of that hack.
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.

8 participants