-
Notifications
You must be signed in to change notification settings - Fork 24.6k
Port index.Tensor
to structured kernels.
#69607
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
Tracking issue: #55070 [ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slow For more information, please take a look at the CI Flow Wiki. |
🔗 Helpful links
✅ No Failures (0 Pending)As of commit 374e448 (more details on the Dr. CI page): Expand to see more💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
Tracking issue: #55070 [ghstack-poisoned]
Tracking issue: #55070 [ghstack-poisoned]
Tracking issue: #55070 [ghstack-poisoned]
index_stub(iter.device_type(), iter, info.indexed_sizes, info.indexed_strides); | ||
at::Tensor res = iter.output(); | ||
|
||
auto result = at::index(self_dq, indices); |
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.
Not sure how important the extra overhead incurred from the dispatcher trip is here. We already know that we're in a QuantizedCPU kernel though, so maybe it's not too unreasonable to call at::cpu::index
? If we do, we should rename the function quantized_index
-> quantized_cpu_index
to avoid confusion.
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.
Ah, you are right. That sounds way more reasonable.
I see a few CI failures. They look like they're mostly coming from unexpected successes in OpInfo tests though. |
DimVector sizes, | ||
DimVector strides, | ||
const Tensor& result) { | ||
index_stub(device_type(), *this, sizes, strides); | ||
} | ||
|
||
Tensor quantized_index(const Tensor & self, const torch::List<c10::optional<Tensor>>& indices) { |
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.
Also, totally not needed but we could swap out the torch::List
here with iOptTensorRefList
while we're at it, to give unboxing quantized calls a small perf boost.
tools/codegen/api/types.py
Outdated
@@ -57,6 +57,7 @@ def __str__(self) -> str: | |||
iOptTensorRefListT = BaseCppType('at', 'IOptTensorRefList') | |||
dimnameT = BaseCppType('at', 'Dimname') | |||
dimnameListT = BaseCppType('at', 'DimnameList') | |||
dimVectorT = BaseCppType('at', 'DimVector') |
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.
this seems reasonable
This looks good :). I'll approve when CI looks good |
Tracking issue: #55070 [ghstack-poisoned]
Tracking issue: #55070 [ghstack-poisoned]
Tracking issue: #55070 [ghstack-poisoned]
Tracking issue: #55070 [ghstack-poisoned]
Tracking issue: #55070 [ghstack-poisoned]
@pytorchbot merge -g |
@pytorchbot successfully started a merge job. Check the current status here |
This diff requires a hot fix in fbcode, but I wasn't able to import it. Hot fix is here, I'll rebase it on top of the internal diff once it enters the diff train: D37015324 |
Merge failed due to 1 additional jobs have failed, first few of them are: Meta Internal-Only Changes Check |
Tracking issue: #55070 [ghstack-poisoned]
@pytorchbot merge -g |
@pytorchbot successfully started a merge job. Check the current status here |
Before the last rebase, all of CI was green except for the "meta internal-only changes check" (which I believe is safe to ignore because this PR was actually created before the GH1 workflow, and I'm planning to abandon the internal diff). Planning to force merge to unblock XLA CI. |
@pytorchbot merge -f |
@pytorchbot successfully started a merge job. Check the current status here |
Hey @ysiraichi. |
Merge failed due to Command
Raised by https://github.com/pytorch/pytorch/actions/runs/2476518424 |
Oh weird--I do think this is not reliable and you can ignore it--the issue tracking it is pytorch/test-infra#397 |
Summary: Tracking issue: #55070 Pull Request resolved: #69607 Approved by: https://github.com/bdhirsh Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/7b3a0ff87a91c98574ddfd98532062d78c49b2c7 Reviewed By: osalpekar Differential Revision: D34706811 Pulled By: bdhirsh fbshipit-source-id: 4146afb5dee63012e27bb92c4b5851daa9c8f6b0
Summary: After landing pytorch/pytorch#69607, that made it an error to use indexing with `cpu_tensor[cuda_indices]`. There was one outstanding test in fbcode that incorrectly used indexing in that way, which is fixed here Reviewed By: bottler, osalpekar Differential Revision: D37128838 fbshipit-source-id: 611b6f717b5b5d89fa61fd9ebeb513ad7e65a656
Stack from ghstack (oldest at bottom):
index.Tensor
to structured kernels. #69607Tracking issue: #55070