[AOTI] Support scalar to tensor in the ABI-compatible mode #118024

desertfire · 2024-01-22T22:37:20Z

Stack from ghstack (oldest at bottom):

-> [AOTI] Support scalar to tensor in the ABI-compatible mode #118024

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @aakhundov @ColinPeppler

Differential Revision: D53019485

[ghstack-poisoned]

pytorch-bot · 2024-01-22T22:37:22Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/118024

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 585e614 with merge base b5b36cf ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler [ghstack-poisoned]

ezyang · 2024-01-23T14:27:59Z

torch/csrc/inductor/aoti_torch/shim_common.cpp

+// AOTI_TORCH_VALUE_TO_TENSOR_IMPL(uint32, uint32_t, UInt32)
+// error: conversion from ‘uint64_t’ {aka ‘long unsigned int’} to
+// ‘torch::detail::TensorDataContainer’ is ambiguous
+// AOTI_TORCH_VALUE_TO_TENSOR_IMPL(uint64, uint64_t, UInt64)


Hmm... Well you could always use scalar_to_tensor instead, that should work...

I see. Let me try that.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler [ghstack-poisoned]

ezyang · 2024-01-23T14:28:26Z

torch/csrc/inductor/aoti_torch/shim_common.cpp


+AtenTensorHandle new_tensor_handle(at::Tensor&& tensor) {
+  at::Tensor* new_tensor = new at::Tensor(std::move(tensor));
+  return tensor_pointer_to_tensor_handle(new_tensor);


Odd, is there a reason this utility didn't exist previously?

It should, but somehow everybody has been just doing copy-pasting. I will post another refactor PR after this.

torch/csrc/inductor/aoti_torch/c/shim.h

ezyang · 2024-01-23T14:29:11Z

torch/_inductor/graph.py

        ] = (
            []
        )  # This is the linemap used by the profiler to mark custom compiled kernels getting run
+        self.symbol_to_dtype: Dict[sympy.Symbol, torch.dtype] = {}


ezyang · 2024-01-23T14:29:41Z

torch/_inductor/ir.py

            self.sym = sym.args[0]
            self.is_bool = True

+        V.graph.symbol_to_dtype[self.sym] = data.get_dtype()


So, I remember you asking me about this, and this is emphatically not what I suggested. Tell me more?

If we return another data struct, say a data class with symbol and dtype, instead of symbol here,

pytorch/torch/_inductor/lowering.py

Line 2558 in e309d6f

return sym

. We need to take care of all the following phases that may expect a symbol type. I am not sure if that's worth to do just for the purpose of this PR.

No, I still think this is very suspicious.

Looking over the modifications you made, the change here is solely so that ShapeAsConstantBuffer can be code generated. But here, there is a strange divergence between the Python in C++. In Python, the shape is code generated as a plain int; however, in C++, the shape is wrapped into a Tensor. This seems... wrong! Now that we are supporting raw integers flowing through AOTInductor, the shape should be represented as is (as a int64_t) when flowing through AOTInductor.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler [ghstack-poisoned]

ghstack-source-id: e265f9f Pull Request resolved: #118024

desertfire · 2024-01-23T21:03:00Z

@desertfire has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ezyang · 2024-01-24T13:41:38Z

torch/_inductor/ir.py

            self.sym = sym.args[0]
            self.is_bool = True

+        V.graph.symbol_to_dtype[self.sym] = data.get_dtype()


No, I still think this is very suspicious.

Looking over the modifications you made, the change here is solely so that ShapeAsConstantBuffer can be code generated. But here, there is a strange divergence between the Python in C++. In Python, the shape is code generated as a plain int; however, in C++, the shape is wrapped into a Tensor. This seems... wrong! Now that we are supporting raw integers flowing through AOTInductor, the shape should be represented as is (as a int64_t) when flowing through AOTInductor.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler Differential Revision: [D53019485](https://our.internmc.facebook.com/intern/diff/D53019485) [ghstack-poisoned]

ghstack-source-id: 6c1f1d7 Pull Request resolved: #118024

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler Differential Revision: [D53019485](https://our.internmc.facebook.com/intern/diff/D53019485) [ghstack-poisoned]

ezyang · 2024-01-25T13:18:11Z

torch/_inductor/codegen/wrapper.py

                if config.aot_inductor.abi_compatible:
+                    output_buffer = V.graph.graph_outputs[idx]
+                    if isinstance(output_buffer, ir.ShapeAsConstantBuffer):
+                        # Need to wrap scalar into tensor as the main function returns a vector of tensors


HMMMM are you ever considering to change this ABI? I can imagine that returning shapes from AOTInductor models is not that common today and so maybe papering over the ABI problem this way is fine, but when we support dynamic shapes AOTInductor models, I really don't want input ints to be passed into the model as tensors, it's soooo unnecessary.

cc @oulgen @bdhirsh

cc @chenyang78 who may look into this later.

ezyang

Thanks, looks good!

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler Differential Revision: [D53019485](https://our.internmc.facebook.com/intern/diff/D53019485) [ghstack-poisoned]

ghstack-source-id: 04aa2cd Pull Request resolved: #118024

desertfire · 2024-01-25T14:27:13Z

@desertfire has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-01-26T03:12:59Z

@pytorchbot merge -f 'Landed internally'

(Initiating merge automatically since Phabricator Diff has merged, using force because this PR might not pass merge_rules.json but landed internally)

pytorchmergebot · 2024-01-26T03:14:56Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

[AOTI] Support scalar to vector in the ABI-compatible mode

7d409c9

[ghstack-poisoned]

desertfire mentioned this pull request Jan 22, 2024

[AOTI] Support .item() in the ABI-compatible mode #117989

Closed

github-actions bot added module: inductor ciflow/inductor labels Jan 22, 2024

desertfire changed the title ~~[AOTI] Support scalar to vector in the ABI-compatible mode~~ [AOTI] Support scalar to tensor in the ABI-compatible mode Jan 23, 2024

Update on "[AOTI] Support scalar to tensor in the ABI-compatible mode"

bba213c

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler [ghstack-poisoned]

desertfire requested a review from ezyang January 23, 2024 14:09

ezyang reviewed Jan 23, 2024

View reviewed changes

Update on "[AOTI] Support scalar to tensor in the ABI-compatible mode"

bceafe8

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler [ghstack-poisoned]

ezyang reviewed Jan 23, 2024

View reviewed changes

desertfire requested a review from ezyang January 23, 2024 15:23

Update on "[AOTI] Support scalar to tensor in the ABI-compatible mode"

2930d4d

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx peterbell10 ipiszy yf225 chenyang78 kadeng muchulee8 aakhundov ColinPeppler [ghstack-poisoned]

desertfire added a commit that referenced this pull request Jan 23, 2024

[AOTI] Support scalar to vector in the ABI-compatible mode

65e3c14

ghstack-source-id: e265f9f Pull Request resolved: #118024

ezyang requested changes Jan 24, 2024

View reviewed changes

ezyang mentioned this pull request Jan 24, 2024

Inductor: KeyError: 'could not find u6' #118218

Closed

desertfire added a commit that referenced this pull request Jan 24, 2024

[AOTI] Support scalar to vector in the ABI-compatible mode

000d034

ghstack-source-id: 6c1f1d7 Pull Request resolved: #118024

ezyang reviewed Jan 25, 2024

View reviewed changes

ezyang approved these changes Jan 25, 2024

View reviewed changes

desertfire added a commit that referenced this pull request Jan 25, 2024

[AOTI] Support scalar to vector in the ABI-compatible mode

901677f

ghstack-source-id: 04aa2cd Pull Request resolved: #118024

pytorchmergebot added the merging label Jan 26, 2024

pytorchmergebot closed this in 4e456fd Jan 26, 2024

pytorchmergebot added Merged and removed merging labels Jan 26, 2024

facebook-github-bot deleted the gh/desertfire/312/head branch January 29, 2024 15:23

chuanqi129 mentioned this pull request Feb 1, 2024

[inductor][cpu]yolov3 fp32 static shape cpp wrapper single thread accuracy crash #118751

Closed

chunyuan-w mentioned this pull request Mar 25, 2024

Inductor cpp wrapper: fix dtype of ShapeAsConstantBuffer #122297

Closed

[AOTI] Support scalar to tensor in the ABI-compatible mode #118024

[AOTI] Support scalar to tensor in the ABI-compatible mode #118024

Uh oh!

Conversation

desertfire commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/118024

✅ No Failures

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

desertfire commented Jan 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

desertfire commented Jan 25, 2024

Uh oh!

facebook-github-bot commented Jan 26, 2024

Uh oh!

pytorchmergebot commented Jan 26, 2024

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

desertfire commented Jan 22, 2024 •

edited

Loading

pytorch-bot bot commented Jan 22, 2024 •

edited

Loading