[Mlir-commits] [mlir] [mlir][memref]: Fix Bug in GlobalOp Verifier (PR #144900)

[Jack Frankland]

FranklandJack wrote:

> This PR assumes that tensor encoding and memref memory space are the same thing. That's not generally the case. Some compilers use that convention during bufferization, but not all do. E.g., the sparse compiler uses the tensor encoding for different purposes.
> 
> I don't quite follow what bug this PR is fixing. Why is it desirable that the tensor attribute encoding and the memref memory space match?

Yeah sorry this issue here is a little bit subtle. To take a concerte example, we have an `arith::ConstantOp` which we want to attach an encoding to. Because `arith::ConstantOp` inherits the `AllTypesMatch<["value", "result"]` interface (or trait?) it needs the initializer type to match the value type of the returned tensor, so both need the encoding. During bufferization we pass the `-use-encoding-for-memory-space` and the `arith::ConstantOp` gets lowered to a `memref::GlobalOp` where the initializer is a tensor type with the encoding value and the operations results type is a memref with the corresponding memory space (these are both integers and happen to match when bufferizing with `-use-encoding-for-memory-space`).

The problem is that [`memref::GlobalOp::verify`](https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp#L1567-L1577) currently requires the types match but doesn't include the memory space :
```
// Check that the type of the initial value is compatible with the type of
    // the global variable.
    if (auto elementsAttr = llvm::dyn_cast<ElementsAttr>(initValue)) {
      Type initType = elementsAttr.getType();
      Type tensorType = getTensorTypeFromMemRefType(memrefType);
      if (initType != tensorType)
        return emitOpError("initial value expected to be of type ")
               << tensorType << ", but was of type " << initType;
    }
  }
 ```
 So `initType` here includes an encoding and `tensorType` doesn't (even though the `memrefType` it mapped from does include an address space which happens to match the encoding in the case of `-use-encoding-for-memory-space` ).
 
 As an example:
 ```
 func.func @test_constant() -> () {
  %0 = "arith.constant"(){value = dense<[42, 24, 13, 52]> : tensor<4xi32, 1 : i32>} : () -> tensor<4xi32, 1 : i32>
  return
}
```
run with:
```
./build-debug/bin/mlir-opt --one-shot-bufferize="use-encoding-for-memory-space" example.mlir
```
with cause a verification error due to the situation described above.

I feel like there might be other solutions here:
1. Relax the `memref::GlobalOp` verifier to ignore the encoding when it compares the reconstructed tensor type against the initializer.
2. Relax the constraint that the types match exactly on `arith::ContantOp` and allow different encodings for the initializer and the returned value.
 

https://github.com/llvm/llvm-project/pull/144900