Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quantized metal. #1594

Closed
wants to merge 11 commits into from
Closed

Quantized metal. #1594

wants to merge 11 commits into from

Conversation

Narsil
Copy link
Collaborator

@Narsil Narsil commented Jan 15, 2024

Working quantized state for candle.

High level overview:

  • Introduce QStorage to split Metal and Cpu ops.
  • Quantize/Dequantize still running on CPU even when asked to run on metal. Kernels exist but led to different quantization/dequantization. GGML removed those kernels on device too (I guess because of the differences).
    I think we should keep the surface for on metal quantize/dequantize so we can easily implement them later. They are part of the ggml API imho.
  • Added a bunch of test, using test_device! in order to get similar testing behavior as regular Tensor.
  • quantized.metal is a direct copy of ggml's ggml-metal.metal. This choice was made so further dev could be made faster and bugs mentionned after can be imported more easily. All the glue logic is in candle_metal_kernels.
  • Introduces a new GgmlDType in candle_metal_kernels. Ggml uses different kernels based on size of matmul and hardware capacity. This wasn't implemented here, but could with the current API.

Worthy bug already discovered (not fixed in this PR since they do not belong here):

  • Q2K Metal -> Bugged (also present in GGML).
  • Q4K CPU -> Bugged (present reviously, new test catch it).
  • Q5K CPU -> Bugged (present previously).
  • Q8_1 Both -> Never really implemented it seems
  • Q8K metal -> Never implemented in metal

Nicolas Patry and others added 9 commits January 15, 2024 17:42
- Add a device param, wherever needed.
- Create new QMetal storage thing that implements QuantizedType.
- Update everywhere needed.

Fix Python.

Fixing examples.

Fix: fmt + clippy + stub.

Moving everything around.

Only missing the actual implems.

Fixing everything + adding dequantized kernels.

More work.

Fixing matmul.

Fmt + Clippy

Some clippy fixes.

Working state.

Q2K Metal -> Bugged (also present in GGML).
Q4K CPU -> Bugged (present previously, new test catch it).
Q5K CPU -> Bugged (present previously).
Q8_1 Both -> Never really implemented it seems
Q8K metal -> Never implemented in metal

Fixing Q2K bug (present in ggml).
@LaurentMazare
Copy link
Collaborator

All the ggmldtype bits seems like an orthogonal refactoring that probably is orthogonal to metal? Could this be split in a separate PR? Also all the fences bits seem orthogonal too and could be extracted.

@LaurentMazare
Copy link
Collaborator

Also blck_size is not a typo, it's on purpose that it matches the llama.cpp nomenclature.

@Narsil
Copy link
Collaborator Author

Narsil commented Jan 17, 2024

All the ggmldtype bits seems like an orthogonal refactoring that probably is orthogonal to metal? Could this be split in a separate PR? Also all the fences bits seem orthogonal too and could be extracted.

No it's not. It's core to it. The reason is that a lot of the previous code was using the GgmlType (the block type, not the dtype) as a generic. This doesn't work for the metal bit since the buffer that actually store the data are untyped (unlike Vec), therefore we need to change that around. (I know we could use PhantomData, but seems very anti-pattern here, and overall the code seems much simpler like this).

For blck_size, I'm ok respecting llama.cpp convention, but to a point, here it's almost silly not writing it all out.

@Narsil
Copy link
Collaborator Author

Narsil commented Jan 17, 2024

Merged #1523 directly.

@Narsil Narsil closed this Jan 17, 2024
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.

2 participants