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

Quantization Verifiers based on T2x set of Traits #2041

Merged
merged 14 commits into from
Mar 5, 2024

Conversation

abhigunj
Copy link
Member

@abhigunj abhigunj commented Feb 23, 2024

  • made isCompatibleElementTypeForHloTypeInference stricter to return error for {not Quantize, Quantize}, {per-axis Quantized, per-tensor Quantized} cases
  • AddOp VHLO Test failures : addressed test failures because {not Quantize, Quantize} is not allowed
  • CorrectedTraits for CholeskyOp and ClampOp to match it with the spec

Note: This PR is based on in review PR #2007

Follow up PR will add/update OP verifiers for OPs which need special handling

@abhigunj abhigunj requested a review from sdasgup3 February 23, 2024 19:31
Copy link
Member

@sdasgup3 sdasgup3 left a comment

Choose a reason for hiding this comment

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

Few initial comments. Will add more related to per-op changes

stablehlo/dialect/Base.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/Base.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/Base.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/Base.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/Base.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.td Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.td Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.td Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.td Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.td Outdated Show resolved Hide resolved
stablehlo/dialect/Base.td Outdated Show resolved Hide resolved
Copy link
Member

@sdasgup3 sdasgup3 left a comment

Choose a reason for hiding this comment

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

Some of the unhandled cases:

  • compare op: Per the spec it uses baseline_element_type(lhs) = baseline_element_type(rhs). However, this check is implemented as

    SameOperandsElementType /*compare_c1*/,
    . How about we add HLO_CompatibleOperandsElementType equivant to
    SameOperandsElementType but work with baseline_element_types.

  • triangular_solve: Use HLO_CompatibleOperandsAndResultElementType instead
    of SameOperandsAndResultElementType.

  • batch_norm_grad, batch_norm_training, and batch_norm_inference: The
    verifier uses AllElementTypesMatch whereas the spec wants the operand to
    matgch using baseline type. Can we introduce HLO_AllElementTypesCompatible.

  • collective_broadcast, collective_permute: They are currently using
    HLO_CompatibleOperandsAndResultType, but per the spec the type checks should
    be implemented using SameOperandsAndResultType

  • reshape: Currently uses HLO_CompatibleOperandsAndResultType but per the
    spec needs stricter type checks using SameOperandsAndResultType.

stablehlo/dialect/Base.cpp Outdated Show resolved Hide resolved
stablehlo/dialect/StablehloOps.td Outdated Show resolved Hide resolved
@abhigunj
Copy link
Member Author

Some of the unhandled cases:

  • compare op: Per the spec it uses baseline_element_type(lhs) = baseline_element_type(rhs). However, this check is implemented as
    SameOperandsElementType /*compare_c1*/,

    . How about we add HLO_CompatibleOperandsElementType equivant to
    SameOperandsElementType but work with baseline_element_types.
  • triangular_solve: Use HLO_CompatibleOperandsAndResultElementType instead
    of SameOperandsAndResultElementType.
  • batch_norm_grad, batch_norm_training, and batch_norm_inference: The
    verifier uses AllElementTypesMatch whereas the spec wants the operand to
    matgch using baseline type. Can we introduce HLO_AllElementTypesCompatible.
  • collective_broadcast, collective_permute: They are currently using
    HLO_CompatibleOperandsAndResultType, but per the spec the type checks should
    be implemented using SameOperandsAndResultType
  • reshape: Currently uses HLO_CompatibleOperandsAndResultType but per the
    spec needs stricter type checks using SameOperandsAndResultType.

Thanks again for the review.
collective_broadcast collective_permute reshape are being handled in follow up PR since existing Trait can't be directly reused for them. Added Traits for other OPs.

@abhigunj abhigunj closed this Feb 28, 2024
@abhigunj abhigunj reopened this Feb 28, 2024
@abhigunj abhigunj force-pushed the t2_verifiers_q_types branch from ffd0da2 to f731558 Compare February 29, 2024 18:50
stablehlo/dialect/Base.h Outdated Show resolved Hide resolved
stablehlo/dialect/Base.h Show resolved Hide resolved
@abhigunj abhigunj assigned GleasonK and unassigned abhigunj Mar 5, 2024
Copy link
Member

@GleasonK GleasonK left a comment

Choose a reason for hiding this comment

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

A few test recommendations to ensure this is all working as expected.

stablehlo/dialect/Base.cpp Show resolved Hide resolved
stablehlo/dialect/Base.cpp Show resolved Hide resolved
@GleasonK GleasonK removed their assignment Mar 5, 2024
@abhigunj abhigunj merged commit da04b39 into openxla:main Mar 5, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants