Skip to content

Conversation

@leslie-fang-intel
Copy link
Collaborator

@leslie-fang-intel leslie-fang-intel commented Nov 26, 2024

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Nov 26, 2024

🔗 Helpful Links

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

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 978d68d with merge base f3d16ec (image):

BROKEN TRUNK - The following job failed but were 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.

[ghstack-poisoned]
@leslie-fang-intel leslie-fang-intel added topic: not user facing topic category ciflow/trunk Trigger trunk jobs on your pull request labels Nov 26, 2024
Copy link
Collaborator

@jgong5 jgong5 left a comment

Choose a reason for hiding this comment

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

Could you please add function doc for the newly added functions.

[ghstack-poisoned]
@leslie-fang-intel
Copy link
Collaborator Author

leslie-fang-intel commented Nov 27, 2024

Could you please add function doc for the newly added functions.

Thanks for the comment. Adding the type hint and doc for these new added functions.

[ghstack-poisoned]
[ghstack-poisoned]
Union[ir.Buffer, ir.ReinterpretView],
]:
"""
Helper function to pre-process out template epilogues for code generation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some comments:

  1. It is more beneficial to explain why we need this function and give a high-level summary of what it does.
  2. After clarify what it does, you will see it is cleaner to move the building of epilogues out of the function to perhaps a dedicated function.
  3. Then, you can consider to give a better name to this function according to the function doc.
Suggested change
Helper function to pre-process out template epilogues for code generation.
The dimension and the indexing could be different between the GEMM output, i.e. `template_buffer`, which is
2D with MxN) and the output from the template after epilogues, i.e. `Y`. In the GEMM template code,
we are not aware of the dimension and the indexing of the epilogues and always work on 2D tiles according to
the indexing of the GEMM output.
In this function, we return a 2D buffer (`Y_2d`) according to GEMM output (reinterpreted from `Y` if needed) and
build a reindexer that converts the indexing of `Y` into `Y_2d`.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the comment. Refactor the implementation by removing epilogues and rename this function to gen_2d_view_of_epilogue_buf.

[ghstack-poisoned]
[ghstack-poisoned]
@leslie-fang-intel
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

pobin6 pushed a commit to pobin6/pytorch that referenced this pull request Dec 5, 2024
…mplate (pytorch#141554)

**Summary**
Extract common internal functions from GEMM Template into public function, so these functions can be reused by the  subsequent group GEMM template.

Pull Request resolved: pytorch#141554
Approved by: https://github.com/jgong5
leslie-fang-intel added a commit to leslie-fang-intel/pytorch that referenced this pull request Dec 9, 2024
@github-actions github-actions bot deleted the gh/leslie-fang-intel/165/head branch December 28, 2024 02:03
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.

5 participants