Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
tannergooding
left a comment
tannergooding
left a comment
There was a problem hiding this comment.
This isn't comprehensive on the failures (it needs to handle all of the All/Any/None APIs).
Additionally, it needs to be done in a way that doesn't pessimize performance. This namely means these should be rewritten to use the *WhereAllBitsSet helpers, such as:
public static bool EqualsAll(Vector2 left, Vector2 right) => Vector2.AllWhereAllBitsSet(Vector2.Equals(left, right));
public static bool EqualsAny(Vector2 left, Vector2 right) => Vector2.AnyWhereAllBitsSet(Vector2.Equals(left, right));This will ensure that we're centralized to a handful of helpers for correctness and then allows us to optimize those couple.
AnyWhereAllBitsSet looks to already be correct, as it's implemented as EqualsAny(vector.AsVector128().AsInt32(), Vector128<int>.AllBitsSet); which will ensure the upper elements of the input are zero, which can't match against the AllBitsSet of the comparison value.
AllWhereAllBitsSet, CountWhereAllBitsSet, IndexOfWhereAllBitsSet, and LastIndexOfWhereAllBitsSet also looks to be already correct. It's implemented as vector.AsVector128().AsInt32() == Vector2.AllBitsSet.AsVector128().AsInt32(); which will ensure the upper elements of both halves are zero and so will match.
NoneWhereAllBitsSet is implemented as !EqualsAny(vector.AsVector128().AsInt32(), Vector128<int>.AllBitsSet); and so is also correct, since the unused elements will never match and so never factor into the comparison.
So the failures here come down to the CmpAny and CmpAll APIs which failed to reuse these general helpers as intended.
There is no issue with All. The issue was with Any which is fixed because Vector128 conversion brings fourth element in Vector3 as default 0 and comparison succeeds. |
it looks like there is, e.g. bool GreaterThanOrEqualAll(Vector2 left, Vector2 right)
=> Vector128.GreaterThanOrEqualAll(left.AsVector128Unsafe(), right.AsVector128Unsafe());AsVector128Unsafe means the last component is undefined so if it's not GreaterThan/Equal while the rest are - the result is false. |
|
updated *All and it's now using AnyWhereAllBitsSet helpers |
|
/ba-g failures are unrelated |
2 similar comments
|
/ba-g failures are unrelated |
|
/ba-g failures are unrelated |
|
/backport to release/10.0 |
|
Started backporting to |
Backport of #123594 to release/10.0 /cc @tannergooding @kasperk81 ## Customer Impact - [x] Customer reported - [ ] Found internally Reported in #123586, developers using some of the new APIs exposed on Vector2 and Vector3 could get non-deterministic results in some scenarios. ## Regression - [ ] Yes - [x] No These are new APIs. ## Testing Explicit tests covering the scenarios were added in addition to manual verification of the codegen. ## Risk Low. These are net new APIs which could accidentally include invalid elements in the accelerated comparison. The fix was to ensure they used the existing centralized helpers that were exposed to help ensure such code was being consistently handled and avoid such problems. The APIs had simply not gotten checked in using them as intended. --------- Co-authored-by: kasperk81 <[email protected]>
4 participants
fix #123586