Implement LLVM x86 SSE4.2 intrinsics #3622
Conversation
TDecking
force-pushed
the
sse4_2
branch
2 times, most recently
from
1563021
to
6ee977c
Compare
|
@eduardosm you've done all the other x86 intrinsics -- can you help review this one? That would be great. :) |
|
@TDecking I see you've done a whole lot of back-and-forth with CI. Is running tests locally not working for you? We have instructions for that here. If that does not work, please describe your problem on Zulip; having to wait for CI adds a lot of friction so we should figure out how to get it all running on your machine. |
|
@eduardosm Thanks for your input. The implementation is fixed and ready. |
|
@eduardosm Ready. |
|
@rustbot ready |
There was a problem hiding this comment.
Thanks!
I can't really review the actual implementations here, as I don't have any clue what they are even trying to implement. 😂 I think in general that's fine, since we have tests -- but to make future maintenance possible here, I ask you to please add a lot more comments. There's a bunch of concepts that are just assumed to be known here, which you have to explain -- like how a string is passed to these functions, or what the difference is between "implicit" and "explicit" (or what these terms even mean).
Also, how good is the test coverage? If someone added a random bug in the middle of one of your functions, would the tests catch that? If not, then it'd be great if you could add more tests, so that when we have to adjust this code in the future (e.g. for Miri API changes) we don't accidentally break it.
| check_shim(unprefixed_name, this, link_name, abi, args)?; | ||
| let mask = compare_strings(this, &str1, &str2, len, imm)?; | ||
|
|
||
| if imm & 0b100_0000 != 0 { |
There was a problem hiding this comment.
What is this comparison about?
There was a problem hiding this comment.
Distinguishing between a bit mask and a byte mask. This comparison is now commented.
| } | ||
|
|
||
| // Used to implement the `_mm_cmpestrm` and the `_mm_cmpistrm` functions. | ||
| // These functions compare the input strings and return the resulting mask. |
| } | ||
|
|
||
| /// The main worker for the string comparison intrinsics, where the given | ||
| /// strings are analyzed according to the given immediate byte. |
There was a problem hiding this comment.
Would be good to briefly say what the arguments mean, and link to the full documentation.
Consider that this will have to be maintained by people that have never seen any of these intrinsics ever before. :) Please help them by leaving comments they can use as starting point should there ever be bugs or other questions around these intrinsics.
src/shims/x86/sse42.rs
Outdated
| Ok(result) | ||
| } | ||
|
|
||
| fn check_shim<'mir, 'tcx: 'mir>( |
There was a problem hiding this comment.
Given that we have a global method called check_shim, please call this something more specific. Also add a doc comment saying what it does, and what all the components in the return type mean.
This seems to be used by all the string functions. So maybe this is a good place to explain what an "input string" even is -- how is the string buffer passed to the function, how is the length determined, etc. Again, assume the reader knows nothing about SSE4.2 (or really, SSE in general), and add enough comments that would give them a basic idea of the structure and goals of the code.
| } | ||
|
|
||
| #[target_feature(enable = "sse4.2")] | ||
| unsafe fn test_crc() { |
There was a problem hiding this comment.
Is there an easily accessible set of test vectors we could use to increase coverage here?
There was a problem hiding this comment.
Not that I am aware of it. I decided to attach an additional test case for each of the intrinsics just in case.
| } | ||
|
|
||
| #[target_feature(enable = "sse4.2")] | ||
| unsafe fn test_str() { |
There was a problem hiding this comment.
Having some tests is a great way to understand what an API does... but here the tests have absolutely no comments so it's not super helpful unfortunately. Is it possible to add some comments that explain what is being tested (at a high level, i.e. for a reader that doesn't know the SSE4.2 functions)?
There was a problem hiding this comment.
I increased the test coverage of the string analysis intrinsics to cover the remaining features offered by them.
These as well as the remaining tests have comments attached to them.
|
@eduardosm thanks a lot for your help with the initial review here! 💛 |
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
@rustbot ready. @RalfJung I've added some additional test cases, which should cover every possible code path now. |
rustbot
added
S-waiting-on-review
SSE4.2 is arguably the least important SIMD extension for the x86 ISA, but it should still be supported for the sake of completeness.