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

[mlir][tablegen] Correctly set Type constraint description #110939

Closed
wants to merge 2 commits into from

Conversation

penagos
Copy link
Member

@penagos penagos commented Oct 2, 2024

The common tablegen Type constraint was not honoring user specified description strings by unconditionally setting Constraint's description to the empty string (this appears to be inadvertent fallout from https://reviews.llvm.org/D94133 in which typeDescription was renamed to description, shadowing the existing description field on Constraint).

This has the effect of making select quantized types (among other things) indistinguishable. For example, quantized types in the TFLite dialect are defined as:

def QUI8 : QuantizedType<"Uniform", [8], 0>;
def QI8 : QuantizedType<"Uniform", [8], 1>;

Where QuantizedType is:

class QuantizedType<string n, list<int> params, bit signed>
  : Type<And<[CPred<"$_self.isa<mlir::quant::QuantizedType>()">,
              CPred<"$_self.cast<mlir::quant::QuantizedType>()" #
                    ".getStorageTypeIntegralWidth() == " # !head(params)>]>,
    "Q" # !if (signed, "I", "UI") # !head(params) # " type"> {
  string name = n;
  string asTraitArgsStr =
    !interleave(params, ", ") # !if(signed, ", true", ", false");
}

Aside from asTraitArgsStr, signed/unsigned quantized types are indistinguishable at the parsed TD level notwithstanding the description argument passed to Type. This fix forwards the user provided description to the underlying Constraint from which Type inherits from.

@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir mlir:ods labels Oct 2, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 2, 2024

@llvm/pr-subscribers-mlir-ods
@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: luis (penagos)

Changes

The common tablegen Type constraint was not honoring user specified description strings by unconditionally setting Constraint's description to the empty string (this appears to be inadvertent fallout from https://reviews.llvm.org/D94133 in which typeDescription was renamed to description, shadowing the existing description field on Constraint).

This has the effect of making select quantized types (among other things) indistinguishable. For example, quantized types in the TFLite dialect are defined as:

def QUI8 : QuantizedType&lt;"Uniform", [8], 0&gt;;
def QI8 : QuantizedType&lt;"Uniform", [8], 1&gt;;

Where QuantizedType is:

class QuantizedType&lt;string n, list&lt;int&gt; params, bit signed&gt;
  : Type&lt;And&lt;[CPred&lt;"$_self.isa&lt;mlir::quant::QuantizedType&gt;()"&gt;,
              CPred&lt;"$_self.cast&lt;mlir::quant::QuantizedType&gt;()" #
                    ".getStorageTypeIntegralWidth() == " # !head(params)&gt;]&gt;,
    "Q" # !if (signed, "I", "UI") # !head(params) # " type"&gt; {
  string name = n;
  string asTraitArgsStr =
    !interleave(params, ", ") # !if(signed, ", true", ", false");
}

Aside from asTraitArgsStr, signed/unsigned quantized types are indistinguishable at the parsed TD level, aside from the description passed to Type. This fix forwards the user provided description to the underlying Constraint from which Type inherits from.


Full diff: https://github.com/llvm/llvm-project/pull/110939.diff

1 Files Affected:

  • (modified) mlir/include/mlir/IR/CommonTypeConstraints.td (+1-1)
diff --git a/mlir/include/mlir/IR/CommonTypeConstraints.td b/mlir/include/mlir/IR/CommonTypeConstraints.td
index 211385245555ad..f5ed8d8a4b0a6b 100644
--- a/mlir/include/mlir/IR/CommonTypeConstraints.td
+++ b/mlir/include/mlir/IR/CommonTypeConstraints.td
@@ -100,7 +100,7 @@ def HasValueSemanticsPred : CPred<"$_self.hasTrait<::mlir::ValueSemantics>()">;
 class Type<Pred condition, string descr = "",
            string cppType = "::mlir::Type"> :
     TypeConstraint<condition, descr, cppType> {
-  string description = "";
+  string description = descr;
   string builderCall = "";
 }
 

@River707
Copy link
Contributor

River707 commented Oct 2, 2024

I'm a little confused by the description of this PR, there is no description field on Constraint. descr corresponds to the summary of the type, not its description.

@penagos
Copy link
Member Author

penagos commented Oct 2, 2024

I'm a little confused by the description of this PR, there is no description field on Constraint. descr corresponds to the summary of the type, not its description.

Ah, I was under the wrong impression from some other unrelated changes I had. You're right description isn't on Constraint (it's summary). Please disregard!

@penagos penagos closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir:ods mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants