[Mlir-commits] [mlir] [mlir][llvm] Fix verifier for const int and dense (PR #74340)

[Rik Huijzer]

https://github.com/rikhuijzer created https://github.com/llvm/llvm-project/pull/74340

Continuation of https://github.com/llvm/llvm-project/pull/74247 to fix https://github.com/llvm/llvm-project/issues/56962. Fixes verifier for (Integer Attr):
```mlir
llvm.mlir.constant(1 : index) : f32
```
and (Dense Attr):
```mlir
llvm.mlir.constant(dense<100.0> : vector<1xf64>) : f32
```

## Integer Attr

The addition that this PR makes to `LLVM::ConstantOp::verify` is meant to be exactly verifying the code in `mlir::LLVM::detail::getLLVMConstant`:

https://github.com/llvm/llvm-project/blob/9f78edbd20ed922cced9482f7791deb9899a6d82/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp#L350-L353

One failure mode is when the `type` (`llvm.mlir.constant(<value>) : <type>`) is not an `Integer`, because then the `cast` in `getIntegerBitWidth` will crash:

https://github.com/llvm/llvm-project/blob/dca432cb7b1c282f5dc861095813c4f40f109619/llvm/include/llvm/IR/DerivedTypes.h#L97-L99

So that's now caught in the verifier.

Apart from that, I don't see anything we could check for. `sextOrTrunc` means "Sign extend or truncate to width" and that one is quite permissive. For example, the following doesn't have to be caught in the verifier as it doesn't crash during `mlir-translate -mlir-to-llvmir`:

```mlir
llvm.func @main() -> f32 {
  %cst = llvm.mlir.constant(100 : i64) : f32
  llvm.return %cst : f32
}
```

## Dense Attr

Crash if not either a MLIR Vector type or one of these:

https://github.com/llvm/llvm-project/blob/9f78edbd20ed922cced9482f7791deb9899a6d82/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp#L375-L391

>From 47140903675a902a5803ef170bcd928783217a5f Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Mon, 4 Dec 2023 16:57:08 +0100
Subject: [PATCH 1/2] [mlir][llvm] Fix verifier for const int

---
 mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp  | 4 ++++
 mlir/test/Target/LLVMIR/llvmir-invalid.mlir | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index 71860363b8ea5..b1c1c2ba04e4c 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -2539,6 +2539,10 @@ LogicalResult LLVM::ConstantOp::verify() {
   if (!llvm::isa<IntegerAttr, ArrayAttr, FloatAttr, ElementsAttr>(getValue()))
     return emitOpError()
            << "only supports integer, float, string or elements attributes";
+  if (auto intAttr = dyn_cast<IntegerAttr>(getValue())) {
+    if (!llvm::isa<IntegerType>(getType()))
+      return emitOpError() << "expected integer constant to have integer type";
+  }
   if (auto floatAttr = dyn_cast<FloatAttr>(getValue())) {
     const llvm::fltSemantics &sem = floatAttr.getValue().getSemantics();
     unsigned floatWidth = APFloat::getSizeInBits(sem);
diff --git a/mlir/test/Target/LLVMIR/llvmir-invalid.mlir b/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
index 0def5895fb330..fb1fb7cf13d7f 100644
--- a/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
@@ -31,6 +31,14 @@ llvm.func @struct_wrong_attribute_element_type() -> !llvm.struct<(f64, f64)> {
 
 // -----
 
+llvm.func @integer_with_float_type() -> f32 {
+  // expected-error @+1 {{expected integer constant to have 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

>From ee0a857ab3fb2e93e51d56d565d29e79f7cdcb67 Mon Sep 17 00:00:00 2001
From: Rik Huijzer <github at huijzer.xyz>
Date: Mon, 4 Dec 2023 17:40:32 +0100
Subject: [PATCH 2/2] Throw error on vector attr with float type

---
 mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp  |  7 ++++++-
 mlir/test/Target/LLVMIR/llvmir-invalid.mlir | 10 +++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index b1c1c2ba04e4c..705788d237d7f 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -2541,7 +2541,7 @@ LogicalResult LLVM::ConstantOp::verify() {
            << "only supports integer, float, string or elements attributes";
   if (auto intAttr = dyn_cast<IntegerAttr>(getValue())) {
     if (!llvm::isa<IntegerType>(getType()))
-      return emitOpError() << "expected integer constant to have integer type";
+      return emitOpError() << "expected integer type";
   }
   if (auto floatAttr = dyn_cast<FloatAttr>(getValue())) {
     const llvm::fltSemantics &sem = floatAttr.getValue().getSemantics();
@@ -2557,6 +2557,11 @@ LogicalResult LLVM::ConstantOp::verify() {
       return emitOpError() << "expected integer type of width " << floatWidth;
     }
   }
+  if (auto splatAttr = dyn_cast<SplatElementsAttr>(getValue())) {
+    if (!getType().isa<VectorType>() && !getType().isa<LLVM::LLVMArrayType>() &&
+        !getType().isa<LLVM::LLVMFixedVectorType>() && !getType().isa<LLVM::LLVMScalableVectorType>())
+      return emitOpError() << "expected vector or array type";
+  }
   return success();
 }
 
diff --git a/mlir/test/Target/LLVMIR/llvmir-invalid.mlir b/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
index fb1fb7cf13d7f..117a6b8269089 100644
--- a/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-invalid.mlir
@@ -7,6 +7,14 @@ func.func @foo() {
 
 // -----
 
+llvm.func @vector_with_non_vector_type() -> f32 {
+  // expected-error @below{{expected vector or array type}}
+  %cst = llvm.mlir.constant(dense<100.0> : vector<1xf64>) : f32
+  llvm.return %cst : f32
+}
+
+// -----
+
 llvm.func @no_non_complex_struct() -> !llvm.array<2 x array<2 x array<2 x struct<(i32)>>>> {
   // expected-error @below{{expected struct type to be a complex number}}
   %0 = llvm.mlir.constant(dense<[[[1, 2], [3, 4]], [[42, 43], [44, 45]]]> : tensor<2x2x2xi32>) : !llvm.array<2 x array<2 x array<2 x struct<(i32)>>>>
@@ -32,7 +40,7 @@ llvm.func @struct_wrong_attribute_element_type() -> !llvm.struct<(f64, f64)> {
 // -----
 
 llvm.func @integer_with_float_type() -> f32 {
-  // expected-error @+1 {{expected integer constant to have integer type}}
+  // expected-error @+1 {{expected integer type}}
   %0 = llvm.mlir.constant(1 : index) : f32
   llvm.return %0 : f32
 }