[PATCH] D75141: [MLIR] Add llvm.mlir.cast op for semantic preserving cast between dialect types.
Alex Zinenko via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 26 01:33:16 PST 2020
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
More information about the llvm-commits
mailing list