Skip to content

Conversation

jiayisunx
Copy link
Collaborator

@jiayisunx jiayisunx commented Mar 27, 2024

Stack from ghstack (oldest at bottom):

Fix #121613.
Modify the output_stride of ConcatKernel: If any input to Concat is Pointwise, check the layout of all inputs to Pointwise, if any of the inputs is in channels_last format, set channels_last strides for the output_stride.

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

Copy link

pytorch-bot bot commented Mar 27, 2024

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit 0877fe9 with merge base 19f5033 (image):

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

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

jiayisunx added a commit that referenced this pull request Mar 27, 2024
ghstack-source-id: 2ab4934
Pull Request resolved: #122761
@jiayisunx jiayisunx marked this pull request as draft March 27, 2024 03:57
jiayisunx added a commit that referenced this pull request Mar 29, 2024
ghstack-source-id: ae1b80d
Pull Request resolved: #122761

Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]

Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]

Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]
jiayisunx added a commit that referenced this pull request Apr 1, 2024
ghstack-source-id: f9a41cf
Pull Request resolved: #122761
@jiayisunx jiayisunx marked this pull request as ready for review April 1, 2024 08:21

Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]
@jiayisunx jiayisunx added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 1, 2024

output_stride = FlexibleLayout.contiguous_strides(new_size)
# If any of the inputs is in CL format, use CL format for the output
any_input_is_storage_and_layout = False
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any_input_is_storage_and_layout = any(is_storage_and_layout(x) for x in inputs) looks clearer?

if isinstance(x, TensorBox):
return is_pointwise_with_channels_last_inputs(x.data)
if isinstance(x, StorageBox) and isinstance(x.data, Pointwise):
all_reads = x.data.get_reads()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar as in #122760, we can get the buffer from read.name. Will it be easy to check the buffer's layout?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that we can share some logic from #122760 too? Both need to get layouts of the input buffers.

if any_input_is_storage_and_layout is False:
if all(
is_pointwise_with_channels_last_inputs(input) for input in inputs
) and len(new_size) in [4, 5]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_pointwise_with_channels_last_inputs should already cover this check? Why check it again here?

return False

return len(all_reads) >= 1 and all(
type(read) is dependencies.MemoryDep and is_channel_last_index(read)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type(read) is dependencies.MemoryDep and is_channel_last_index(read)
isinstance(read, dependencies.MemoryDep) and is_channels_last_index(read)

output_stride = make_channels_last_strides_for(new_size)
break
if any_input_is_storage_and_layout is False:
if all(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explain why we requires "all" if none of the inputs is storage_and_layout instead of "any" on any input is storage_and_layout.

if isinstance(x, TensorBox):
return is_pointwise_with_channels_last_inputs(x.data)
if isinstance(x, StorageBox) and isinstance(x.data, Pointwise):
all_reads = x.data.get_reads()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that we can share some logic from #122760 too? Both need to get layouts of the input buffers.

jiayisunx added a commit that referenced this pull request Apr 7, 2024
ghstack-source-id: 489aadd
Pull Request resolved: #122761

Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]

Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]

Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]
jiayisunx added a commit that referenced this pull request Apr 10, 2024
ghstack-source-id: ba12c0e
Pull Request resolved: #122761

Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]
jiayisunx added a commit that referenced this pull request Apr 10, 2024
ghstack-source-id: 505425f
Pull Request resolved: #122761
@leslie-fang-intel
Copy link
Collaborator

LGTM, just a few more comments.

fx_node_args = [
fx_node_args,
]
if any_input_is_storage_and_layout is False and any(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest to add a note here explaining the heuristics we use here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, thanks!


Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]

Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]
jiayisunx added a commit that referenced this pull request Apr 15, 2024
ghstack-source-id: 525e8ef
Pull Request resolved: #122761

Fix #121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.



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

[ghstack-poisoned]
@jiayisunx jiayisunx requested a review from jansel April 17, 2024 01:26
@jiayisunx
Copy link
Collaborator Author

@jansel , could you please review this PR? Thanks.

@jiayisunx
Copy link
Collaborator Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@jiayisunx
Copy link
Collaborator Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

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

sanketpurandare pushed a commit to sanketpurandare/pytorch that referenced this pull request Apr 22, 2024
Fix pytorch#121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.

Pull Request resolved: pytorch#122761
Approved by: https://github.com/jgong5, https://github.com/leslie-fang-intel, https://github.com/jansel
petrex pushed a commit to petrex/pytorch that referenced this pull request May 3, 2024
Fix pytorch#121613.
Modify the `output_stride` of `ConcatKernel`: If any input to `Concat` is `Pointwise`, check the layout of all inputs to `Pointwise`, if any of the inputs is in channels_last format, set channels_last strides for the `output_stride`.

Pull Request resolved: pytorch#122761
Approved by: https://github.com/jgong5, https://github.com/leslie-fang-intel, https://github.com/jansel
@github-actions github-actions bot deleted the gh/jiayisunx/5/head branch May 31, 2024 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants