Skip to content

Conversation

johannes-graner
Copy link
Contributor

Proposed changes

Pre-commit changed a lot of files unrelated to changes made in #2894, I separated those changes into this separate PR.
This change makes the pre-commit hook run without issue on CK Tile.

Checklist

Please put an x into the boxes that apply. You can also fill these out after creating the PR. If you're not sure, please don't hesitate to ask.

  • I have added tests relevant to the introduced functionality, and the unit tests are passing locally
  • I have added the test to REGRESSION_TESTS list defined at the top of CMakeLists.txt in tests/CMakeLists.txt, IF the test takes more than 30 seconds to run.
  • I have added inline documentation which enables the maintainers with understanding the motivation
  • I have removed the stale documentation which is no longer relevant after this pull request
  • (If this change is user-facing) I have added release notes which provide the end users with a brief summary of the improvement from this pull request
  • I have run clang-format on all changed files
  • Any dependent changes have been merged

@johannes-graner johannes-graner force-pushed the jograner/ck_tile/pre_commit_remod branch from 6444ebc to ef66996 Compare September 22, 2025 14:01
@johannes-graner johannes-graner force-pushed the jograner/ck_tile/pre_commit_remod branch from ef66996 to f80f3dc Compare September 22, 2025 14:06
@bartekxk bartekxk requested a review from Copilot September 22, 2025 14:12
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR runs pre-commit reformatting on the CK Tile codebase to address code style consistency issues. The changes are primarily automatic reformatting applied by pre-commit hooks to ensure the codebase follows consistent style guidelines.

  • Updates the Python remod.py script to use consistent string quoting and formatting
  • Adds missing header includes to various operation header files for proper module organization
  • Reorders includes alphabetically in some files

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.

Show a summary per file
File Description
include/ck_tile/remod.py Converts single quotes to double quotes and improves formatting consistency
include/ck_tile/ops/*.hpp Adds missing common header includes (load_interleaved_pk_type.hpp, streamk_common.hpp)
include/ck_tile/ops/common.hpp Reorders includes alphabetically
include/ck_tile/ops/gemm.hpp Reorders multi_d_kernel include alphabetically
include/ck_tile/host.hpp Adds missing permute_pk_int4.hpp include

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

bartekxk
bartekxk previously approved these changes Sep 22, 2025
Copy link
Contributor

@DDEle DDEle left a comment

Choose a reason for hiding this comment

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

Should we run pre-commit including remod.py in out CI pipeline?

ck_tile::HostTensor<BDataType> b_k_n_dev = b_k_n;
// permute_tensor_b<decltype(b_k_n_dev)>(b_k_n_dev);
permute_vectors_i4x4_b(b_k_n_dev);
gemm_common::permute_vectors_i4x4_b(b_k_n_dev);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use function from ck_tile namespace?

Copy link
Contributor Author

@johannes-graner johannes-graner Sep 26, 2025

Choose a reason for hiding this comment

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

I think they do different permutations, at least according to comments (AFAICT comments are correct):
gemm_common.hpp: 01234567 -> 20643175
permute_vectors_i4x4_b.hpp: 0x76543210 -> 0x75316420
So I don't think they are interchangable. I do think it's a bit misleading to have two functions called the same thing but doing different permutations, so if you have a suggestion for alternative names I'm all ears!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants