[PATCH] D75141: [MLIR] Add llvm.mlir.cast op for semantic preserving cast between dialect types.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

Makes sense to me, mostly nits.



================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:689
 
+def LLVM_MLIRCastOp : LLVM_Op<"mlir.cast", [NoSideEffect]>,
+                      Results<(outs AnyType:$res)>,
----------------
Nit: drop "MLIR" from the def name, we are not using it in other operations (`constant`, `undef`)


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:704
+  }];
+  let assemblyFormat = [{ $in attr-dict `:` type($in) `->` type($res) }];
+  let verifier = "return ::verify(*this);";
----------------
Nit: the implicit convention for cast operations is to use `type($in) 'to' type($res)`, let's follow it here as well.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:1820
+        typeConverter.convertType(castOp.getType())) {
+      castOp.emitError() << "illegal llvm.mlir.cast conversion";
+      return matchFailure();
----------------
Nit: I'd say that the llvm.mlir.cast itself isn't legal, not the conversion. 

Actually, I'm not convinced we want to report an error from the pattern at all.  There may be cases where a partial conversion is sufficient and the caller will deal with the remaining casts properly, yet we would have reported the error to the user by that time....


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:898
+static LogicalResult verify(MLIRCastOp op) {
+  auto verifyMLIRCastType = [&op](Type type) -> LogicalResult {
+    if (auto llvmType = type.dyn_cast<LLVM::LLVMType>()) {
----------------
Don't you also want to support memrefs?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75141/new/

https://reviews.llvm.org/D75141