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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:294
+static LogicalResult verify(InvokeOp op) {
+
+  if (op.getNumResults() > 1)
----------------
Remove the newline.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:300
+
+  if (op.getSuccessor(1)->getOperations().size() == 0)
+    return op.emitError(
----------------
Never use 'size' for operation/block/region lists, it is not O(1). You should be able to use getSuccessor(1)->empty() instead.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:372
+
+  auto funcType = type.dyn_cast<FunctionType>();
+  if (!funcType)
----------------
This should already be checked by parseType, so no need to duplicate error handling. You should be able to replace uses of 'funcType' with 'type' directly.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:440
+  // Clauses
+  for (auto it : op.getOperands()) {
+    // Similar to llvm - if clause is an array type then it is filter
----------------
nit: auto -> Value


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:459
+  UnitAttr ua;
+  parser.parseAttribute(ua, "cleanup", result.attributes);
+
----------------
You need to check this parse method for failure.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:462
+  // Parse clauses with types
+  while ((parser.parseOptionalLParen() || true) &&
+         (!parser.parseOptionalKeyword("filter") ||
----------------
The || true seems off, what invariant are you trying to capture here? Also, it is good practice to explicitly wrap optional parse methods with 'succeeded' or 'failed' to make the intent clear.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:478
+  if (parser.parseColon() || parser.parseType(type))
+    return parser.emitError(typeLocation, "invalid type");
+
----------------
You shouldn't emit error for these methods, an error will already be emitted.


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:523
+
+    for (unsigned i = 0; i < lpi->getNumClauses(); i++) {
+      ops.push_back(processConstant(lpi->getClause(i)));
----------------
Remove trivial braces. Also, please cache the end iterator to avoid recomputation.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:228
+      else
+        return emitError(opInst.getLoc(), "constant clauses expected");
+    }
----------------
Is this error case captured by any of the tests? I can't seem to find where it's covered.


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

https://reviews.llvm.org/D72006