🔗 Helpful Links

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

• 📄 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 3b3f56e with merge base e6e140c ():
💚 Looks good so far! There are no failures yet. 💚

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

nit: do we need to add assert here

pytorch/torch/_inductor/codegen/cpp_micro_gemm.py

Line 518 in d1bb8e8

for (int64_t n = 0; n < N; n += {{block_n}}) {

that for WOQ int8, N == block_n

In the CPP template, it's being done for all dtypes - while calling a micro-kernel, cache blocking size for N dimension is equal to the register blocking size for N. I added a comment in the code, so that we may make necessary changes (buffer allocation & index computation) if that'd cease to be the case.

Thanks!

@pytorchbot rebase

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

Successfully rebased cache_b_tiles_int8_woq_gemm onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout cache_b_tiles_int8_woq_gemm && git pull --rebase)

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

Merge failed

Reason: 1 jobs have failed, first few of them are: inductor-periodic / cuda12.1-py3.10-gcc9-sm86-periodic-dynamo-benchmarks / test (aot_eager_torchbench, 2, 2, linux.g5.4xlarge.nvidia.gpu)

@pytorchbot merge

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

@pytorchbot revert -m "correctness issue in #140208"

❌ 🤖 pytorchbot command failed:

@pytorchbot revert: error: the following arguments are required: -c/--classification
usage: @pytorchbot revert -m MESSAGE -c
                          {nosignal,ignoredsignal,landrace,weird,ghfirst}

Try @pytorchbot --help for more info.

@pytorchbot revert -m "correctness issue in #140208" -c nosignal

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

Reverting PR 136688 failed

Reason: Command git -C /home/runner/work/pytorch/pytorch revert --no-edit 3cbf0c0bbf7567816a3a2893d59b1d9bc936a90f returned non-zero exit code 1

Auto-merging test/inductor/test_cpu_select_algorithm.py
Auto-merging torch/_inductor/codegen/cpp_gemm_template.py
Auto-merging torch/_inductor/codegen/cpp_micro_gemm.py
CONFLICT (content): Merge conflict in torch/_inductor/codegen/cpp_micro_gemm.py
error: could not revert 3cbf0c0bbf7... [Inductor][CPP] Cache weight tiles in L1D for AMX int8 WoQ GEMM (#136688)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git revert --continue".
hint: You can instead skip this commit with "git revert --skip".
hint: To abort and get back to the state before "git revert",
hint: run "git revert --abort".
hint: Disable this message with "git config advice.mergeConflict false"

1. This PR seems caused correctness issue as reported in [Inductor][CPP] CPP GEMM Template WOQ int8 correctness failure #140208
2. I would like to discuss more about the implementation in this PR. Considering here:

   pytorch/torch/_inductor/codegen/cpp_micro_gemm.py

   Line 572 in e2e425b

   load_dequantized_B(n);

   we are dealing with Matrix B of size Kc * Nr which should be contiguous after packing, why not we just create a dequantized_B_buf with same size/stride but different data type. Then we can dequant the elements from B into dequantized_B_buf with exactly same index. Written a POC here for this idea: leslie-fang-intel@4c83e4e. It can pass the correctness check of above 2 failed test case and seems simplifying the implementation in this PR.

Also, it seems we need more UTs to cover this feature...

Also, it seems we need more UTs to cover this feature...

An existing UT caught it :(
We need CI coverage of AMX

1. This PR seems caused correctness issue as reported in [Inductor][CPP] CPP GEMM Template WOQ int8 correctness failure #140208

Thanks for the info! The size of the buffer should not have been multiplied with 2

we are dealing with Matrix B of size `Kc * Nr` which should be contiguous after packing, why not we just create a `dequantized_B_buf` with same size/stride but different data type. Then we can dequant the elements from `B` into `dequantized_B_buf` with exactly same index. Written a POC here for this idea: [leslie-fang-intel@4c83e4e](https://github.com/leslie-fang-intel/pytorch/commit/4c83e4e75138e8fa6e0d58438f75b7718dc8a0cc). It can pass the correctness check of above 2 failed test case and seems simplifying the implementation in this PR.

Thanks! The patch assumes that all elements of a B tile are contiguous in memory, as opposed to being a tile like this -

Are elements of B tiles copied to some intermediate buffer, so that they can be accessed contiguously? Thanks!

@chunyuan-w explained that the weights are being pre-packed to let each tile being accessed contiguously. So, I'm guessing when @jgong5 will add N != {{block_n}} implementation in the micro-kernel, then two consecutive tiles would be used. I'll revise the patch accordingly. Thanks!