[PATCH] D71888: [mlir] Added llvm.resume in LLVM IR Dialect

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

Thanks for working on this! Please feel free to add me as a reviewer when you are have `landingpad` implemented.



================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:457
+  let verifier = [{
+    if(getNumOperands() != 1)
+      return emitOpError("expected exactly two successors");
----------------
The number of operands should be checked by declaring them in the "arguments" list with appropriate types. This will need another check to make sure the type matches that mentioned in some "landingpad", but that check can assume the "arguments"-based check passed.

Note that in MLIR, operation verifiers are not allowed to inspect sibling operations (since operations can be processed in parallel). So the type-match verifier should be implemented on the enclosing operation, likely in LLVMFuncOp.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:762
+static void printResumeOp(OpAsmPrinter &p, ResumeOp &op) {
+  assert(op.getNumOperands() == 1);
+  p << op.getOperationName();
----------------
This should be enforced by the verifier. Generally, we don't check the verifier-enforced properties in functions.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:772
+
+  if (parser.parseOperandList(operands) || operands.empty() ||
+      parser.parseColonType(type) ||
----------------
If there's strictly one operand, I'd suggest using parser.parseOperand instead. It would provide a more meaningful message in case the operand is absent.


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

https://reviews.llvm.org/D71888