support memset on 128-bit integers#595
Conversation
f2e86c0 to
fbb18af
Compare
There was a problem hiding this comment.
A couple nits - I am already testing a resolution in personal PR: brody2consult#1 - now resolved ✅
I will push the updates to this PR if I do not encounter any failures in brody2consult#1 - now done ✅
e8b869c to
ae65a4c
Compare
ae65a4c to
4afcd1b
Compare
|
I've generalized it to work with arbitrary bitpatterns and on signed and unsigned 128 bit integers. If you could confirm this patch works for you, I'd be happy to merge it. I wrote this into the compiletest:
Generally, we do accept patches that do enable you to use crates that previously didn't compile, even if you can't use the u128bit path. In similar vein, we've added |
Co-authored-by: firestar99 <firestar99@vectorware.com>
4afcd1b to
b4308b4
Compare
|
I just force-pushed another update to update the test comments, use "128-bit" with hyphen in the title, and add you as co-author. I changed the link to link to the code that actually failed to compile. Good to go from my end assuming that the tests continue to succeed. I have also updated the PR description to resolve issue #594. I do think it would be ideal to show a build warning in case of compiling with non-working functionality, in a similar fashion to "warn on large non-zero fills" as proposed in PR #586. But I would be happy to defer this for now. I did try the following code mutations which do not trigger any failures in compiletests: diff --git a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs
index 1bea4fedfe..29667d8ed3 100644
--- a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs
+++ b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs
@@ -309,8 +309,8 @@ fn memset_fill_u64(b: u8) -> u64 {
}
fn memset_fill_u128(b: u8) -> u128 {
- let b64 = memset_fill_u64(b) as u128;
- b64 | b64 >> 64
+ let b64 = memset_fill_u32(b) as u128;
+ b64 >> 20
}
fn memset_dynamic_scalar(I don't think this should be any issue since this is non-working functionality anyways. But I had wanted to find a way to test my own work on this as discussed in #596. Thanks for your quick handling of this contribution; I am extremely happy overall. |
resolves #594
This is a quick solution to support the case I described in #594:
This proposal would help me get past the first build issue I have encountered with RustCrypto.
I would love to support non-zero memset fill for both
u128&i128as discussed in #594, but this would involve some more work in testing to hopefully ensure that this is done correctly & not likely broken by potential future refactoring.I would be happy to address any possible feedback on my first contribution to Rust-GPU.
TODO
squash commits before merging - Reviewers please let me know if you want me to do this or if you want to do this yourself