Skip to content

[mlir][polynomial] Add and verify constraints of coefficientModulus for ringAttr #111016

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

Merged

Conversation

ZenithalHourlyRate
Copy link
Member

Currently the semantic of coefficientModulus is unclear and a lowering of it faces uncertainty, for example, google/heir#995 (comment)

Also, it lacks a verifier which should conform to the definition in the document.

This PR tries to further define the semantic of coefficientModulus and adds a verifier for it.

Cc @j2kun for review and suggestions.

@llvmbot llvmbot added the mlir label Oct 3, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 3, 2024

@llvm/pr-subscribers-mlir

Author: Hongren Zheng (ZenithalHourlyRate)

Changes

Currently the semantic of coefficientModulus is unclear and a lowering of it faces uncertainty, for example, google/heir#995 (comment)

Also, it lacks a verifier which should conform to the definition in the document.

This PR tries to further define the semantic of coefficientModulus and adds a verifier for it.

Cc @j2kun for review and suggestions.


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

3 Files Affected:

  • (modified) mlir/include/mlir/Dialect/Polynomial/IR/PolynomialAttributes.td (+12)
  • (modified) mlir/lib/Dialect/Polynomial/IR/PolynomialAttributes.cpp (+29)
  • (modified) mlir/test/Dialect/Polynomial/attributes.mlir (+34)
diff --git a/mlir/include/mlir/Dialect/Polynomial/IR/PolynomialAttributes.td b/mlir/include/mlir/Dialect/Polynomial/IR/PolynomialAttributes.td
index 2d3ed60a35fd9f..7d59add3d37c2b 100644
--- a/mlir/include/mlir/Dialect/Polynomial/IR/PolynomialAttributes.td
+++ b/mlir/include/mlir/Dialect/Polynomial/IR/PolynomialAttributes.td
@@ -161,6 +161,17 @@ def Polynomial_RingAttr : Polynomial_Attr<"Ring", "ring"> {
 
     The coefficient and polynomial modulus parameters are optional, and the
     coefficient modulus is only allowed if the coefficient type is integral.
+
+    The coefficient modulus, if specified, should be positive and not larger
+    than `2 ** width(coefficientType)`.
+
+    If the coefficient modulus is not specified, the handling of coefficients
+    overflows is determined by subsequent lowering passes, which may choose to
+    wrap around or widen the overflow at their discretion.
+
+    Note that coefficient modulus is contained in `i64` by default, which is signed.
+    To specify a 64 bit number without intepreting it as a negative number, its container
+    type should be manually specified like `coefficientModulus=18446744073709551615:i128`.
   }];
 
   let parameters = (ins
@@ -168,6 +179,7 @@ def Polynomial_RingAttr : Polynomial_Attr<"Ring", "ring"> {
     OptionalParameter<"::mlir::IntegerAttr">: $coefficientModulus,
     OptionalParameter<"::mlir::polynomial::IntPolynomialAttr">: $polynomialModulus
   );
+  let genVerifyDecl = 1;
   let assemblyFormat = "`<` struct(params) `>`";
   let builders = [
     AttrBuilderWithInferredContext<
diff --git a/mlir/lib/Dialect/Polynomial/IR/PolynomialAttributes.cpp b/mlir/lib/Dialect/Polynomial/IR/PolynomialAttributes.cpp
index 93c7f9e687fc7c..cd7789a2e9531c 100644
--- a/mlir/lib/Dialect/Polynomial/IR/PolynomialAttributes.cpp
+++ b/mlir/lib/Dialect/Polynomial/IR/PolynomialAttributes.cpp
@@ -203,5 +203,34 @@ Attribute FloatPolynomialAttr::parse(AsmParser &parser, Type type) {
   return FloatPolynomialAttr::get(parser.getContext(), result.value());
 }
 
+LogicalResult
+RingAttr::verify(function_ref<mlir::InFlightDiagnostic()> emitError,
+                 Type coefficientType, IntegerAttr coefficientModulus,
+                 IntPolynomialAttr polynomialModulus) {
+  if (coefficientModulus) {
+    auto coeffIntType = llvm::dyn_cast<IntegerType>(coefficientType);
+    if (!coeffIntType) {
+      return emitError() << "coefficientModulus specified but coefficientType "
+                            "is not integral";
+    }
+    APInt coeffModValue = coefficientModulus.getValue();
+    if (coeffModValue == 0) {
+      return emitError() << "coefficientModulus should not be 0";
+    }
+    if (coeffModValue.slt(0)) {
+      return emitError() << "coefficientModulus should be positive";
+    }
+    auto coeffModWidth = (coeffModValue - 1).getActiveBits();
+    auto coeffWidth = coeffIntType.getWidth();
+    if (coeffModWidth > coeffWidth) {
+      return emitError() << "coefficientModulus needs bit width of "
+                         << coeffModWidth
+                         << " but coefficientType can only contain "
+                         << coeffWidth << " bits";
+    }
+  }
+  return success();
+}
+
 } // namespace polynomial
 } // namespace mlir
diff --git a/mlir/test/Dialect/Polynomial/attributes.mlir b/mlir/test/Dialect/Polynomial/attributes.mlir
index 4bdfd44fd4d159..cb3216900cb430 100644
--- a/mlir/test/Dialect/Polynomial/attributes.mlir
+++ b/mlir/test/Dialect/Polynomial/attributes.mlir
@@ -37,3 +37,37 @@
 // expected-error@below {{failed to parse Polynomial_RingAttr parameter 'coefficientModulus' which is to be a `::mlir::IntegerAttr`}}
 // expected-error@below {{expected attribute value}}
 #ring1 = #polynomial.ring<coefficientType=i32, coefficientModulus=x, polynomialModulus=#my_poly>
+
+// -----
+
+// expected-error@below {{coefficientModulus specified but coefficientType is not integral}}
+#ring1 = #polynomial.ring<coefficientType=f32, coefficientModulus=17>
+
+// -----
+
+// expected-error@below {{coefficientModulus should not be 0}}
+#ring1 = #polynomial.ring<coefficientType=i32, coefficientModulus=0>
+
+// -----
+
+// expected-error@below {{coefficientModulus should be positive}}
+#ring1 = #polynomial.ring<coefficientType=i32, coefficientModulus=-1>
+
+// -----
+
+// expected-error@below {{coefficientModulus needs bit width of 33 but coefficientType can only contain 32 bits}}
+#ring1 = #polynomial.ring<coefficientType=i32, coefficientModulus=4294967297>
+
+// -----
+
+#ring1 = #polynomial.ring<coefficientType=i32, coefficientModulus=4294967296>
+
+// -----
+
+// expected-error@below {{coefficientModulus should be positive}}
+#ring1 = #polynomial.ring<coefficientType=i64, coefficientModulus=18446744073709551615>
+
+// -----
+
+// unfortunately, coefficientModulus of 64bit should be contained in larger type
+#ring1 = #polynomial.ring<coefficientType=i64, coefficientModulus=18446744073709551615 : i128>

@j2kun j2kun merged commit 4425dfb into llvm:main Oct 5, 2024
10 checks passed
Kyvangka1610 pushed a commit to Kyvangka1610/llvm-project that referenced this pull request Oct 5, 2024
…or ringAttr (llvm#111016)

Currently the semantic of coefficientModulus is unclear and a lowering
of it faces uncertainty, for example,
google/heir#995 (comment)

Also, it lacks a verifier which should conform to the definition in the
document.

This PR tries to further define the semantic of coefficientModulus and
adds a verifier for it.

Cc @j2kun for review and suggestions.
Kyvangka1610 added a commit to Kyvangka1610/llvm-project that referenced this pull request Oct 5, 2024
* commit 'FETCH_HEAD':
  [clang][bytecode] Handle UETT_OpenMPRequiredSimdAlign (llvm#111259)
  [mlir][polynomial] Add and verify constraints of coefficientModulus for ringAttr (llvm#111016)
  [clang][bytecode] Save a per-Block IsWeak bit (llvm#111248)
  [analyzer] Fix wrong `builtin_*_overflow` return type (llvm#111253)
  [SelectOpt] Don't convert constant selects to branches. (llvm#110858)
  [InstCombine] Update and-or-icmps.ll after 574266c. NFC
  [Instcombine] Test for more gep canonicalization
  [NFC][TableGen] Change `CodeGenIntrinsics` to use const references (llvm#111219)
  Add warning message to `session save` when transcript isn't saved. (llvm#109020)
  [RISCV][TTI] Recognize CONCAT_VECTORS if a shufflevector mask is multiple insert subvector. (llvm#110457)
  Revert "[InstCombine] Folding `(icmp eq/ne (and X, -P2), INT_MIN)`" (llvm#111236)
  [NFC][lsan] Add SuspendAllThreads traces
  [lsan] Add `thread_suspend_fail` flag

Signed-off-by: kyvangka1610 <[email protected]>
ZenithalHourlyRate added a commit to ZenithalHourlyRate/heir that referenced this pull request Oct 7, 2024
Check llvm/llvm-project#111016

More constraints on coefficientModulus are defined so tests
are updated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants