[PATCH] D72006: [MLIR] Added llvm.invoke and llvm.landingpad

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:337
+      result.addOperands(ops);
+      result.addAttribute("callee", callee);
+      result.addSuccessor(normal, normalOps);
----------------
nit: just invoke the other build method after adding the 'callee' attribute.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:301
+    return op.emitError(
+        "must have atleast one operation in unwind destination");
+
----------------
atleast -> at least


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:303
+
+  // If the first instruction is not LandingpadOp
+  if (!isa<LandingpadOp>(*op.getSuccessor(1)->begin()))
----------------
This sentence seems incomplete.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:304
+  // If the first instruction is not LandingpadOp
+  if (!isa<LandingpadOp>(*op.getSuccessor(1)->begin()))
+    return op.emitError("first operation in unwind destination should be a "
----------------
->front()?


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:338
+
+// <operation> ::= `llvm.invoke` (function-id | ssa-use) `(` ssa-use-list `)`
+//                  `to` bb-id (`[` ssa-use-and-type-list `]`)?
----------------
Use /// for top-level comments.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:399
+    argTypes.reserve(funcType.getNumInputs());
+    for (int i = 0, e = funcType.getNumInputs(); i < e; ++i) {
+      auto argType = funcType.getInput(i).dyn_cast<LLVM::LLVMType>();
----------------
nit: Use range based for loop instead: funcType.getInputs()


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:411
+    auto funcArguments =
+        ArrayRef<OpAsmParser::OperandType>(operands).drop_front();
+
----------------
nit: llvm::makeArrayRef(operands).drop_front()


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:423
+  }
+  result.addSuccessor(normalDest, normalOperands);
+  result.addSuccessor(unwindDest, unwindOperands);
----------------
nit: Can you hoist these above the 'if (isDirect)' branch so that you can change that to early exit instead?


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:485
+  // Check for cleanup
+  UnitAttr ua;
+  if (succeeded(parser.parseOptionalKeyword("cleanup")))
----------------
'ua' looks unused.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:491
+  while (succeeded(parser.parseOptionalLParen()) &&
+         (succeeded(parser.parseOptionalKeyword("filter")) ||
+          succeeded(parser.parseOptionalKeyword("catch")))) {
----------------
Are the 'catch'/'filter' truly optional? i.e. should we emit an error if neither are present?


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:226
+      // All operands should be constant - checked by verifier
+      if (auto constOper = dyn_cast<llvm::Constant>(operand))
+        lpi->addClause(constOper);
----------------
nit: constOperand


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

https://reviews.llvm.org/D72006