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

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added a comment.
This revision now requires changes to proceed.

You are missing some tests.

Simple comments for now, haven't had time to think much about the modeling.

For 2. I would remove the attributes and just do the same thing that LLVM does, i.e. if the type is an Array it is a filter. I don't see a reason why attributes are necessary on top of that.



================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:350
+    if (getNumSuccessors() != 2)
+      return emitOpError("must have a success and failure successor");
+    for(auto &inst : *getSuccessor(1)) {
----------------
Can we use the same names as LLVM for 'success' and 'failure'?


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:352
+    for(auto &inst : *getSuccessor(1)) {
+      // FIXME: Ignore all phi inst and check for landingpad
+    }
----------------
This should be checked now, we don't model PHIInst like LLVM does so this should be even simpler.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:444
+    : LLVM_OneResultOp<"landingpad", [NoSideEffect]>,
+      Arguments<(ins BoolAttr:$cleanup, StrArrayAttr:$clauseTypes, Variadic<LLVM_Type>:$clauses)> {
+  let parser = [{ return parseLandingPadOp(parser, result); }];
----------------
Lines should be wrapped at 80 characters just like code.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:433
+
+// <operation> ::= `llvm.call` (function-id | ssa-use) `(` ssa-use-list `)`
+//                 attribute-dict? `:` function-type
----------------
This needs to be updated


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:436
+static ParseResult parseInvokeOp(OpAsmParser &parser, OperationState &result) {
+  SmallVector<NamedAttribute, 4> attrs;
+  SmallVector<OpAsmParser::OperandType, 8> operands;
----------------
Why is this necessary? Why not just use result.attributes directly instead?


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:442
+  SmallVector<Value, 4> failureOperands;
+  Type type;
+  SymbolRefAttr funcAttr;
----------------
Change this to FunctionType, `parseType` will already check that the parsed type isa<FunctionType>


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:791
+  p << ' ';
+  p.printOperands(op.getOperands());
+  p << " : ";
----------------
Stream these in:

p << ' ' << op.getOperands() << " : " << op.getType();


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:798
+                                     OperationState &result) {
+  llvm::dbgs() << "Parsing LandingPadOp\n";
+  return success();
----------------
This seems unimplemented


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:199
 Attribute Importer::getConstantAsAttr(llvm::Constant *value) {
+  value->dump();
   if (auto *ci = dyn_cast<llvm::ConstantInt>(value))
----------------
Leftover debugging.


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:204
         ci->getValue());
-  if (auto *c = dyn_cast<llvm::ConstantDataArray>(value))
-    if (c->isString())
+  if (auto *c = dyn_cast<llvm::ConstantDataArray>(value)) {
+    if (c->isString()) {
----------------
This looks unrelated.


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:538
+    ops.reserve(inst->getNumOperands());
+    for (auto &op : invokeInst->arg_operands()) {
+      ops.push_back(processValue(op.get()));
----------------
Remove all trivial braces.


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:557
+    llvm::BasicBlock *unwindDest = invokeInst->getUnwindDest();
+    state.addSuccessor(blocks[normalDest],
+                       processBranchArgs(invokeInst, normalDest));
----------------
Please use proper builder.create<> instead, adding build methods to Invoke and LandingPad as necessary.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:204
+                           blockMapping[invOp.getSuccessor(1)], operandsRef);
+      // builder.CreateCall(functionMapping.lookup(attr.getValue()),
+      // operandsRef);
----------------
Remove all commented out code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72006