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

[Tim Shen via Phabricator via llvm-commits]

timshen marked an inline comment as done.
timshen added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:689
 
+def LLVM_MLIRCastOp : LLVM_Op<"mlir.cast", [NoSideEffect]>,
+                      Results<(outs AnyType:$res)>,
----------------
ftynse wrote:
> Nit: drop "MLIR" from the def name, we are not using it in other operations (`constant`, `undef`)
Well it has to be named as "SomethingCast", as LLVM_CastOp already exists. Renamed to "DialectCast".


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:1820
+        typeConverter.convertType(castOp.getType())) {
+      castOp.emitError() << "illegal llvm.mlir.cast conversion";
+      return matchFailure();
----------------
ftynse wrote:
> 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....
Good point. Removed the error reporting.


================
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>()) {
----------------
ftynse wrote:
> Don't you also want to support memrefs?
memref is complicated because of the descriptor. Do we allow the conversion between a memref and the aligned base pointer, or a memref and the descriptor? Neither seems trivial to implement to me, nor I really need it at the moment. Added a TODO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75141