-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fix inaccurate std::intrinsics::simd
documentation
#137828
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe outside the scope of this PR, but this would need to be checked in codegen and emit an ICE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what exactly? you get a monomorphization error when these rules are violated. They are not the prettiest errros, but they are not ICEs either. Unless I'm missing something. e.g. https://godbolt.org/z/f5j5re5z8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's a post-mono error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good. I misunderstood and thought you meant the intrinsic was permitting invalid behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is just bringing the docs up to date with the behavior that we already have and enforce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this in the LLVM backend I don't even see such a widening cast. This code converts the masks to
i1
vectors which LLVM seems to use for them, and it does that withlshr
andtrunc
. Thelshr
is entirely unnecessary for correctness as we require the input to be all-1 or all-0, butBut I can't find anything that would go wrong with unsigned integers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally agree, in that an intrinsic (that normal users should never have to worry about), we could either demand that the lane width matches the other arguments, or that the mask uses sign extension no matter the signedness of the argument.
From what I can tell the codegen backends already handle this, because e.g. in llvm (and from what i can see, also cranelift and gcc) the integers are just a bunch of bits.
The counter-argument was that performing sign extension on what rust believes is a vector with unsigned types would violate type safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what you mean by that. There's no sign extension happening, as far as I can tell. Also, all the backends have to do is implement the intended mask semantics, which is described in a bitwise way. If they use sign extension as part of that, that's completely fine. There's no type safety violation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, the backends support it, and the intrinsic could (and in my opinion should) support it too. I'm just relaying the response I got when I suggested exactly that https://rust-lang.zulipchat.com/#narrow/channel/257879-project-portable-simd/topic/add.20.60simd_max.60.20and.20.60simd_min.60/near/502647748
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this discussion to Zulip: https://rust-lang.zulipchat.com/#narrow/channel/257879-project-portable-simd/topic/On.20the.20sign.20of.20masks