-
Notifications
You must be signed in to change notification settings - Fork 487
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
Re-land: Make as_strided_copy
materialize a new tensor with index
.
#6697
Conversation
I will test for the regression described here on the GPU machine I have access to. |
dynamo issue can be fixed by rebasing, fine to ignore. |
84ec60b
to
1a204f7
Compare
@lsy323 Could you help me checking if the regression is gone? |
Do we need this pr in the 2.3 release? It is a rather dangerous change, if we don;t have a strong reason I'd rather leave it in nightly for now. |
@vanbasten23 can you please help @ysiraichi benchmark this fix on TPU and confirm perf outcome? @JackCaoG given the risk, I'd be ok we leave this PR out for 2.3 |
yea, unless there is a strong reason I would prefer to leave this out of 2.3 releas. |
Do we have bandwidth to test this one? Otherwise we can merge and see if DDP test started to fail tmr.... |
I'm running the tests in #6624 (comment). |
@ysiraichi sorry for the delayed response. I tested on my v3-8. Before this PR (master branch 6ac3223):
With the PR:
I don't see any slowdown. The change lgtm. Thanks Yukio. |
Thanks, @vanbasten23. |
Re-land: #6624
This PR adds a fast path on top of #6624 changes.
Fast path: keep old behavior of
as_strided_copy
Slow path: new behavior
cc @miladm @JackCaoG @lsy323