Skip to content

Cx/instruction level acc sim - #15

Open
firemountain154B wants to merge 8 commits into
mainfrom
cx/non-linear-acc-sim
Open

Cx/instruction level acc sim#15
firemountain154B wants to merge 8 commits into
mainfrom
cx/non-linear-acc-sim

Conversation

@firemountain154B

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread .specific.sh

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would it be good to ignore this file?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes

@jiaeenie

jiaeenie commented Jul 14, 2025

Copy link
Copy Markdown
Collaborator

A general question: why do we quantize the outputs of matmul, RMSNorm, softmax, and SiLU and should we also include the output of Embedding layer, and ROPE to minifloat here? Is it to simulate the fact that activations are written to Vector Core SRAM in minifloat, and then later fetched and cast to MXFP as input(which we had so far)? Would @ChengZhang-98 be able to help me to validate if it is the correct behaviour in the simulator? Thanks a lot!

@jiaeenie
jiaeenie requested a review from ChengZhang-98 July 14, 2025 22:06
enable_eval_harness: Whether to run evaluation via EleutherAI lm-eval-harness.
"""
preset_mxfp_X, preset_mxfp_W, preset_mxfp_Kv, preset_minifloat_NL = validate_and_sanitize_quant_args(
preset_mxfp_X, preset_mxfp_W, preset_mxfp_Kv, preset_minifloat_X, preset_minifloat_NL = validate_and_sanitize_quant_args(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we introduce a new flag here, I think preset_minifloat_NL here sets the minifloat format for all nonlinear ops.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

the minifloat_x is actually not the minifloat_NL, I think this behaviour should be merged into X quant, cause in hardware, if you have a matmul after matmul (projection after ov in attention), the intermediate result will also be stored in vector sram which is minifloat.

if func_type =="Xq":
assert x_minifp_meta is not None, "MinifloatMeta must be provided for 'Xq' input"
input = minifloat_ieee_quantizer(input, x_minifp_meta)
quantizer = partial(minifloat_ieee_quantizer, meta=x_minifp_meta)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shall we make the hardware-aware quantization optional in the simulation framework, while keeping the previous coarser-grained input quantization as the default?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

to be honest, this is not that hardware related, this is instruction-aware quantization (I didn't expose those tiny hardware mismatch in this code), so in this sense, if we just quantize the input and output of non-linear, the acc will have a really big accuracy mismatch with real hardware, which unacceptable I guess

@@ -0,0 +1,187 @@
from typing import Literal, Union

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could see the new changes in main for config parsing.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

sorry my bad, the new changes looks better, but can we include the preset_minifloat_X stuff in the the config parsing

@@ -10,14 +10,15 @@ def _minifloat_ieee_quantize(x: Tensor, meta: MinifloatMeta) -> Tensor:
mantissa_bits = meta.element_frac_bits
exponent_bias = meta.exponent_bias

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I need some help here, would it be okay for @ChengZhang-98 to review this file?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Note here: the -1 to -2 is for describing the hardware behaviour, we're still leaving one bit for inf.

the later one is to handle the situation when we set config to higher bits, the quantizer will turn inf to nan

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

May I ask why we keep Inf instead of saturation for minifloat in hardware?

@firemountain154B firemountain154B Jul 15, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is related to the hardware design itself, in 4 bit exponent case, bias is 4'b0111, which represents 7, the inf is 1111,
while doing some operation on exponent, the current design will take exponent bit out and tansfer it to the unbiased exponent. the unbiased exponent will be a signed integer.

In the inf case, 1111 - 0111 = 1000, if we still want to operate in 4 bit, the signed 4'b1000 actually means -8 ( but we want it to be 8).
There are several design choices to deal with this, for example, rewrite all the addition, or increase one bit. but in this case, we choose to keep the inf bit, cause 1. this is for minifloat(we're possible to have more bit on this), we can have another version for lower bit-width case (mxfp), 2. to align with IEEE standard. 3. don't want to break the design.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A question here, it doesn't seem like the hardware_aware_operations are exposing dse precision related parameters such as FP_ADD_EXT_EXP_WIDTH etc. More specifically the ones defined in this config file here.
@GeorgeWu1204 Please correct me if I'm wrong

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, please just ignore these parameters for now; I will update them later.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

So in this, I'd say this is not that hardware related, just instruction aware

@@ -10,14 +10,15 @@ def _minifloat_ieee_quantize(x: Tensor, meta: MinifloatMeta) -> Tensor:
mantissa_bits = meta.element_frac_bits
exponent_bias = meta.exponent_bias

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

May I ask why we keep Inf instead of saturation for minifloat in hardware?

# f"Legal values are: {legal_scale_exp_bits}."
# )

legal_element_exp_frac_bits = ((4, 3), (5, 2), (2, 3), (3, 2), (2, 1))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is dangerous. The MXFP quantizer in mase-triton does not aim to support all the possible (scale bit, exp bit, frac bit) setups, which is impossible for the limited number of dtypes in pytorch/GPU.

Current quantizer in mase-triton assumes the input data is BF16. FP32/FP64 numbers will be cast to BF16 implicitly before converting to MXFP.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

got it

)
import re

# 提取 m4e3 中的 4 和 3

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove this, or use Eng.

What does cc in the file name mean?

@firemountain154B firemountain154B Jul 15, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

some historical reason,, jiayi made a new set_quant_args, but I'm not awaring this, so we might want to keep jiayi's new quant_args, so then random put this file here.. will be removed later

enable_eval_harness: Whether to run evaluation via EleutherAI lm-eval-harness.
"""
preset_mxfp_X, preset_mxfp_W, preset_mxfp_Kv, preset_minifloat_NL = validate_and_sanitize_quant_args(
preset_mxfp_X, preset_mxfp_W, preset_mxfp_Kv, preset_minifloat_X, preset_minifloat_NL = validate_and_sanitize_quant_args(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could someone elaborate the difference between preset_minifloat_X and preet_minifloat_NL?
Do we need both?

btw, I think docstrings are needed to explain the meaning of these notations.

in terms of naming, probably N is better than NL? We use KV because that's two tensors, key and value, if we want, we can support separate precisions for K and V, but NL is just non-linear tensor?

OCP_MXFP8_E4M3,
OCP_MXFP8_E5M2,
)
import re

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

probably move this import of built-in package to the top of the file

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

got it

@@ -0,0 +1,13 @@

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this related to acc-sim?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

will be removed

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