[EISW-191805] Enable quantization for FP4 type #175
Conversation
Jacenty-And-Intel
changed the title
|
Can this be first brought to upstream MLIR? Also, does it by any chance conflict with what @Roman-Pevnyi is trying to do in upstream? (see llvm/llvm-project#152966) |
@andrey-golubev Wanted to merge it in the fork first. Right now enabling PSS tests for FP4 and further work on quantization to f4 are blocked without the change in MLIR.
It does not conflict, in fact, it works very well with Roman's change. Once that change is approved, enabling FP4 will be much simpler, and enabling further types will be even easier. However, we don't know when it will be merged upstream, and it is currently a blocker on our side. |
|
Do you have a testing PR in vpux-plugin that verifies CI when updating this submodule, to be linked here? |
I would strongly prefer to do it the other way around since merging to upstream is usually a painful process. Quantization changes are also evidently painful: the dialect evolved quite a bit since LLVM 18 up to LLVM 21 (we're now at LLVM 20).
I think this is now nice to do, but we also have automated tests running in CI! (See https://github.com/intel/npu-plugin-llvm/actions/runs/19365101483/job/55416928201?pr=175) |
andrey-golubev
left a comment
andrey-golubev
left a comment
There was a problem hiding this comment.
Looks good to me. And it also looks fairly non-controversial. Please try to bring it up to the upstream community first!
Generally, it seems like most of the checks would benefit from isa<FloatType> kind of check. We just need 1 place somewhere (e.g. in verifier?) where we explicitly check that types are f4 / f8.
P.S.: Not approving yet - perhaps @sartil or @ZoranZomborat may want to take a look
| const size_t typeWidthSize = 1 << typeWidth; | ||
| const size_t expectedSize = | ||
| (storageTypeRange < typeWidthSize) ? storageTypeRange : typeWidthSize; | ||
| (storageTypeRange < typeWidthSize) && !mlir::isa<FloatType>(storageType) ? storageTypeRange : typeWidthSize; |
There was a problem hiding this comment.
this looks like a separate bug fix. why is it required?
| } else if (storageType.isa<Float8E5M2Type>() || | ||
| storageType.isa<Float8E4M3FNType>()) { | ||
| // Both Float8E5M2Type and Float8E4M3FNType derive from FloatType. | ||
| } else if (mlir::isa<Float8E5M2Type, Float8E4M3FNType, Float4E2M1FNType>(storageType)) { |
There was a problem hiding this comment.
nitpick:
| } else if (mlir::isa<Float8E5M2Type, Float8E4M3FNType, Float4E2M1FNType>(storageType)) { | |
| } else if (mlir::isa<FloatType>(storageType)) { |
this seems sufficient here.
| } else if (llvm::dyn_cast<Float8E5M2Type>(type) || | ||
| llvm::dyn_cast<Float8E4M3FNType>(type)) { | ||
| storageTypeWidth = 8; | ||
| } else if (mlir::isa<Float8E5M2Type, Float8E4M3FNType, Float4E2M1FNType>(type)) { |
There was a problem hiding this comment.
ditto. just cast to FloatType?
3 participants
Summary
After the vpux type is added for float4_e2m1, quantization should be enabled for this type, as it is for FP8. This MLIR change will allow for further adjustments to enable FP4 quantization in the compiler and enable the addition of PSS tests.
JIRA ticket
Related PR in NPU Compiler and/or OpenVINO repository with sub-module update
Other related tickets