[mlir][llvm dialect] Verify element type of nested types #148975

newling · 2025-07-15T22:11:34Z

Before this PR, this was valid

 %0 = llvm.mlir.constant(dense<[1, 2]> : vector<2xi32>) : vector<2xf32>

but this was not:

%0 = llvm.mlir.constant(1 : i32) : f32

because only scalar types were checked for compatibility, not the element types of nested types.

Another additional check that this PR adds is to verify the float semantics. Before this PR,

 %cst = llvm.mlir.constant(1.0 : bf16) : f16

was considered valid (because bf16 and f16 both have 16 bits), but with this PR it is not considered valid.

This PR also moves all tests on the verifier of the llvm constant op into a single file.

The total number of tests with this PR increases by 3.

To summarize the state after this PR. Valid:

Invalid:

%0 = llvm.mlir.constant(dense<[128, 1024]> : vector<2xi32>) : vector<2xf32>
%0 = llvm.mlir.constant(dense<[128., 1024.]> : vector<2xbf16>) : vector<2xf16>

Valid:

%0 = llvm.mlir.constant(dense<[128., 1024.]> : vector<2xf32>) : vector<2xi32>
%0 = llvm.mlir.constant(dense<[128, 1024]> : vector<2xi64>) : vector<2xi8>

and identical valid/invalid cases for the scalar cases.

llvmbot · 2025-07-15T22:12:06Z

@llvm/pr-subscribers-mlir

Author: James Newling (newling)

Changes

Move some tests from Target/LLVMIR/llvmir-invalid.mlir to Dialect/LLVMIR/invalid.mlir so that all tests of ConstantOp::verify() are in the same file.

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

2 Files Affected:

(modified) mlir/test/Dialect/LLVMIR/invalid.mlir (+25)
(modified) mlir/test/Target/LLVMIR/llvmir-invalid.mlir (-24)

diff --git a/mlir/test/Dialect/LLVMIR/invalid.mlir b/mlir/test/Dialect/LLVMIR/invalid.mlir
index bd1106e304c60..e5fe78c077314 100644
--- a/mlir/test/Dialect/LLVMIR/invalid.mlir
+++ b/mlir/test/Dialect/LLVMIR/invalid.mlir
@@ -439,6 +439,31 @@ llvm.func @scalable_vec_requires_splat() -> vector<[4]xf64> {
   llvm.return %0 : vector<[4]xf64>
 }
 
+
+// -----
+
+llvm.func @integer_with_float_type() -> f32 {
+  // expected-error @+1 {{expected integer type}}
+  %0 = llvm.mlir.constant(1 : index) : f32
+  llvm.return %0 : f32
+}
+
+// -----
+
+llvm.func @incompatible_float_attribute_type() -> f32 {
+  // expected-error @below{{expected float type of width 64}}
+  %cst = llvm.mlir.constant(1.0 : f64) : f32
+  llvm.return %cst : f32
+}
+
+// -----
+
+llvm.func @incompatible_integer_type_for_float_attr() -> i32 {
+  // expected-error @below{{expected integer type of width 16}}
+  %cst = llvm.mlir.constant(1.0 : f16) : i32
+  llvm.return %cst : i32
+}
+
 // -----
 
 func.func @insertvalue_non_llvm_type(%a : i32, %b : i32) {
diff --git a/mlir/test/Target/LLVMIR/llvmir-invalid.mlir b/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
index a8ef401fff27e..6c7a218d0676e 100644
--- a/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
@@ -55,30 +55,6 @@ llvm.func @struct_wrong_attribute_element_type() -> !llvm.struct<(f64, f64)> {
 
 // -----
 
-llvm.func @integer_with_float_type() -> f32 {
-  // expected-error @+1 {{expected integer type}}
-  %0 = llvm.mlir.constant(1 : index) : f32
-  llvm.return %0 : f32
-}
-
-// -----
-
-llvm.func @incompatible_float_attribute_type() -> f32 {
-  // expected-error @below{{expected float type of width 64}}
-  %cst = llvm.mlir.constant(1.0 : f64) : f32
-  llvm.return %cst : f32
-}
-
-// -----
-
-llvm.func @incompatible_integer_type_for_float_attr() -> i32 {
-  // expected-error @below{{expected integer type of width 16}}
-  %cst = llvm.mlir.constant(1.0 : f16) : i32
-  llvm.return %cst : i32
-}
-
-// -----
-
 // expected-error @below{{LLVM attribute 'readonly' does not expect a value}}
 llvm.func @passthrough_unexpected_value() attributes {passthrough = [["readonly", "42"]]}

llvmbot · 2025-07-15T22:12:07Z

@llvm/pr-subscribers-mlir-llvm

Author: James Newling (newling)

Changes

Move some tests from Target/LLVMIR/llvmir-invalid.mlir to Dialect/LLVMIR/invalid.mlir so that all tests of ConstantOp::verify() are in the same file.

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

2 Files Affected:

(modified) mlir/test/Dialect/LLVMIR/invalid.mlir (+25)
(modified) mlir/test/Target/LLVMIR/llvmir-invalid.mlir (-24)

diff --git a/mlir/test/Dialect/LLVMIR/invalid.mlir b/mlir/test/Dialect/LLVMIR/invalid.mlir
index bd1106e304c60..e5fe78c077314 100644
--- a/mlir/test/Dialect/LLVMIR/invalid.mlir
+++ b/mlir/test/Dialect/LLVMIR/invalid.mlir
@@ -439,6 +439,31 @@ llvm.func @scalable_vec_requires_splat() -> vector<[4]xf64> {
   llvm.return %0 : vector<[4]xf64>
 }
 
+
+// -----
+
+llvm.func @integer_with_float_type() -> f32 {
+  // expected-error @+1 {{expected integer type}}
+  %0 = llvm.mlir.constant(1 : index) : f32
+  llvm.return %0 : f32
+}
+
+// -----
+
+llvm.func @incompatible_float_attribute_type() -> f32 {
+  // expected-error @below{{expected float type of width 64}}
+  %cst = llvm.mlir.constant(1.0 : f64) : f32
+  llvm.return %cst : f32
+}
+
+// -----
+
+llvm.func @incompatible_integer_type_for_float_attr() -> i32 {
+  // expected-error @below{{expected integer type of width 16}}
+  %cst = llvm.mlir.constant(1.0 : f16) : i32
+  llvm.return %cst : i32
+}
+
 // -----
 
 func.func @insertvalue_non_llvm_type(%a : i32, %b : i32) {
diff --git a/mlir/test/Target/LLVMIR/llvmir-invalid.mlir b/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
index a8ef401fff27e..6c7a218d0676e 100644
--- a/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
@@ -55,30 +55,6 @@ llvm.func @struct_wrong_attribute_element_type() -> !llvm.struct<(f64, f64)> {
 
 // -----
 
-llvm.func @integer_with_float_type() -> f32 {
-  // expected-error @+1 {{expected integer type}}
-  %0 = llvm.mlir.constant(1 : index) : f32
-  llvm.return %0 : f32
-}
-
-// -----
-
-llvm.func @incompatible_float_attribute_type() -> f32 {
-  // expected-error @below{{expected float type of width 64}}
-  %cst = llvm.mlir.constant(1.0 : f64) : f32
-  llvm.return %cst : f32
-}
-
-// -----
-
-llvm.func @incompatible_integer_type_for_float_attr() -> i32 {
-  // expected-error @below{{expected integer type of width 16}}
-  %cst = llvm.mlir.constant(1.0 : f16) : i32
-  llvm.return %cst : i32
-}
-
-// -----
-
 // expected-error @below{{LLVM attribute 'readonly' does not expect a value}}
 llvm.func @passthrough_unexpected_value() attributes {passthrough = [["readonly", "42"]]}

gysit

Thanks a lot for improving the verifiers!

I have added some small nit comments but otherwise looks good to go.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

gysit · 2025-07-16T06:05:12Z

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

+    type = arrayType.getElementType();
+  if (auto vecType = dyn_cast<VectorType>(type))
+    return vecType.getElementType();
+  if (auto tenType = dyn_cast<TensorType>(type))


nit: the TensorType can be dropped. It is not a compatible LLVM type (see isCompatibleOuterType) and since ConstantOp returns an LLVM type there should be no tensors here.

Should the following fail to verify?

%0 = llvm.mlir.constant(dense<[1.0, 1.0]> : tensor<2xf64>) : vector<2xf64>

It currently roundtrips without issue.

Maybe what is confusing here is that I'm using this method to test both the attribute's element type as well the constant op's result's element type?

I would have expected that we use:

%0 = llvm.mlir.constant(dense<[1.0, 1.0]> : vector<2xf64>) : vector<2xf64>

But if the tensor type shows up in the wild on the attribute then we can keep your version of the code.

gysit · 2025-07-16T06:20:46Z

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

+      if (!constantElementType.isInteger(floatWidth)) {
+        return emitOpError() << "expected integer type of width " << floatWidth;
+      }


Suggested change

if (!constantElementType.isInteger(floatWidth)) {

return emitOpError() << "expected integer type of width " << floatWidth;

}

if (!constantElementType.isInteger(floatWidth))

return emitOpError() << "expected integer type of width " << floatWidth;

nit: for single line ifs we usually drop the braces

gysit · 2025-07-16T06:21:07Z

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

+    if (auto floatType = dyn_cast<FloatType>(attrElmType)) {
+      return verifyFloatSemantics(floatType.getFloatSemantics(), resultElmType);
+    }


Suggested change

if (auto floatType = dyn_cast<FloatType>(attrElmType)) {

return verifyFloatSemantics(floatType.getFloatSemantics(), resultElmType);

}

if (auto floatType = dyn_cast<FloatType>(attrElmType))

return verifyFloatSemantics(floatType.getFloatSemantics(), resultElmType);

nit: Same as above

This looks like a leftover?

gysit · 2025-07-16T06:25:00Z

mlir/test/Dialect/LLVMIR/invalid.mlir

+  llvm.return %0 : !llvm.struct<(f64, f64)>
+}
+
+// -----


Suggested change

// -----

nit: one split can be dropped

gysit · 2025-07-16T06:27:16Z

mlir/test/Dialect/LLVMIR/invalid.mlir

+// -----
+
+llvm.func @int_attr_requires_int_type() -> f32 {
+  // expected-error @+1 {{expected integer type}}


Suggested change

// expected-error @+1 {{expected integer type}}

// expected-error @below {{expected integer type}}

ulta nit: can you use below everywhere in the moved tests? It is preferred over @+1

newling · 2025-07-16T18:00:36Z

@gysit Thank you for reviewing this! I think I have addressed your comments. The one I'm not sure about is the removal of TensorType from the new static function, getElementType. Please see my inline comment.

gysit

Thanks for addressing the comment!

LGTM modulo some leftover nit.

I did one more search and realized that the tensor type is also used for llvm.array constants. That is actually a somewhat justified use case. Let's keep your current version of the code then!

gysit · 2025-07-17T06:37:11Z

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

+    if (auto floatType = dyn_cast<FloatType>(attrElmType)) {
+      return verifyFloatSemantics(floatType.getFloatSemantics(), resultElmType);
+    }


This looks like a leftover?

move them to the correct place

b04ad6e

llvmbot added mlir:llvm mlir labels Jul 15, 2025

newling changed the title ~~[mlir][llvm dialect][NFC] Ensure tests of constant op verifier to common file~~ [mlir][llvm dialect][NFC] Put tests of constant op verifier in common file Jul 15, 2025

newling marked this pull request as draft July 16, 2025 00:50

add checks on element attributes for types

77f4673

newling changed the title ~~[mlir][llvm dialect][NFC] Put tests of constant op verifier in common file~~ [mlir][llvm dialect] Verify element type of nested types Jul 16, 2025

newling marked this pull request as ready for review July 16, 2025 01:10

gysit reviewed Jul 16, 2025

View reviewed changes

newling added 2 commits July 16, 2025 10:10

address a few review comments

68be7ae

use original error message (as per review comment)

f229840

gysit approved these changes Jul 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[mlir][llvm dialect] Verify element type of nested types #148975

[mlir][llvm dialect] Verify element type of nested types #148975

Uh oh!

newling commented Jul 15, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jul 15, 2025

Uh oh!

llvmbot commented Jul 15, 2025

Uh oh!

gysit left a comment

Uh oh!

Uh oh!

gysit Jul 16, 2025

Uh oh!

newling Jul 16, 2025

Uh oh!

gysit Jul 17, 2025

Uh oh!

gysit Jul 16, 2025

Uh oh!

gysit Jul 16, 2025

Uh oh!

gysit Jul 17, 2025

Uh oh!

gysit Jul 16, 2025

Uh oh!

gysit Jul 16, 2025

Uh oh!

newling commented Jul 16, 2025

Uh oh!

gysit left a comment

Uh oh!

gysit Jul 17, 2025

Uh oh!

Uh oh!

	// expected-error @+1 {{expected integer type}}
	// expected-error @below {{expected integer type}}

[mlir][llvm dialect] Verify element type of nested types #148975

Are you sure you want to change the base?

[mlir][llvm dialect] Verify element type of nested types #148975

Uh oh!

Conversation

newling commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 15, 2025

Uh oh!

llvmbot commented Jul 15, 2025

Uh oh!

gysit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newling commented Jul 16, 2025

Uh oh!

gysit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

newling commented Jul 15, 2025 •

edited

Loading