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

[shraiysh via Phabricator via llvm-commits]

shraiysh added inline comments.


================
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 `]`)?
----------------
rriddle wrote:
> Use /// for top-level comments.
// was used for other functions in the file. Should I change it for all of them for uniformity?


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:423
+  }
+  result.addSuccessor(normalDest, normalOperands);
+  result.addSuccessor(unwindDest, unwindOperands);
----------------
rriddle wrote:
> nit: Can you hoist these above the 'if (isDirect)' branch so that you can change that to early exit instead?
`addSuccessor` internally is appending successor operands to `result.operands` and therefore, adding successor before adding operands leads to unexpected failures.


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


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

https://reviews.llvm.org/D72006