[PATCH] D72006: [MLIR] Added llvm.invoke and llvm.landingpad
shraiysh via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 22 11:10:07 PST 2020
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
More information about the llvm-commits
mailing list