[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:462
+  // Parse clauses with types
+  while ((parser.parseOptionalLParen() || true) &&
+         (!parser.parseOptionalKeyword("filter") ||
----------------
rriddle wrote:
> 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.
Actually I was trying to make parenthesis optional - but they had to be parsed if present. Hence they `|| true`. Now, I realize that code in the current patch would lead to valid parsing of `( catch... (catch... (filter...` also (which is wrong). So, in the new patch, I am making parenthesis compulsory.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:228
+      else
+        return emitError(opInst.getLoc(), "constant clauses expected");
+    }
----------------
rriddle wrote:
> Is this error case captured by any of the tests? I can't seem to find where it's covered.
I have added tests for invalid IR in `test/Dialect/LLVMIR/invalid.mlir`. Also, I have added a verified for landingpad instruction


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

https://reviews.llvm.org/D72006