-
Notifications
You must be signed in to change notification settings - Fork 696
Add check for negative index in cpu kernel op_index #15366
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/15366
Note: Links to docs will display an error until the docs builds have been completed. ❌ 9 New Failures, 2 Pending, 4 Unrelated FailuresAs of commit b2d4503 with merge base de56c81 ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but was present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
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.
looks fine; inline comments are for your consideration and not blocking. However, please also add a test for this behavior (and feel free to @ me for re-review to avoid extra delay).
Fast path cannot be taken when the index is negative in op_index. It results in a failure in `check_fast_path_args`. Add a check to `check_fast_path_conditions` for negative index and return `false` if one is encountered. Change-Id: I6c3fb1793da9a0831ef41a47dc2ab4423aa38be3 Signed-off-by: Sebastian Larsson <[email protected]>
0315d9b to
b2d4503
Compare
|
Changed to your suggestions and added tests @swolchok |
| EXPECT_TENSOR_EQ(ret, expected); | ||
| } | ||
|
|
||
| TEST_F(OpIndexTensorOutTest, NegativeIndexSupportedForInt) { |
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 would prefer to see a templated test, but it's probably not worth blocking on
|
looks like we have a bunch of infra failures and known flakes in CI. merging. |
Fast path cannot be taken when the index is negative in op_index. It results in a failure in
check_fast_path_args. Add a check tocheck_fast_path_conditionsfor negative index and returnfalseif one is encountered.cc @freddan80 @per @zingo @oscarandersson8218 @digantdesai