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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Please add proper builders. Users should *not* have to use OperationState directly, it is extremely error prone and clunky.



================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:333
+  let verifier = [{
+    if(getNumResults() > 1)
+      return emitOpError("must have 0 or 1 result");
----------------
Please move large blocks of c++ into the .cpp file. 


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:349
+    
+    if(!lpOp) {
+      return emitOpError("No landingpad instruction in unwind branch");
----------------
Remove trivial braces. There are still quite a few remaining.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:357
+      // Check it must be an operand
+      for(auto ops : lpOp.getOperands())
+        if(ops.getDefiningOp() == prevInst)
----------------
This loop seems fairly inefficient. Can you rework this?


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:363
+        return emitOpError("first instruction in unwind branch should be"
+                      "landingpad instruction");
+    }
----------------
This seems slightly incorrect. Other instructions are allowed if they feed into the landingpad.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:431
+    bool isArrayTy = it.getType().cast<LLVMType>().isArrayTy();
+    p << (isArrayTy ? "filter " : "catch ") << it.getType() << ' ' << it << ' ';
+  }
----------------
Can you print this in a more MLIR friendly way? i.e. operand : type


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:448
+  // Check for cleanup
+  if (parser.parseOptionalKeyword("cleanup"))
+    result.addAttribute("cleanup", parser.getBuilder().getBoolAttr(false));
----------------
Can we use a UnitAttr for this instead? 


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:462
+      return failure();
+    tys.push_back(ty);
+    operands.push_back(operand);
----------------
Why not just resolve the operand right here?


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:471
+
+  parser.getCurrentLocation(&typeLocation);
+  if (parser.parseColon() || parser.parseType(type)) {
----------------
I don't see where the location is used.


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:546
+    if (llvm::Function *callee = ii->getCalledFunction()) {
+      opState.addAttribute(b.getIdentifier("callee"),
+                           b.getSymbolRefAttr(callee->getName()));
----------------
nit: You can elide the b.getIdentifier() and just pass the string name.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:223
+    // Only one type - that is going to be lpType
+    for (auto res : opInst.getResultTypes()) {
+      lpType = &res;
----------------
This looks error prone, what is the invariant you want here?


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:232
+    // Add clauses
+    auto operands = lookupValues(lpOp.getOperands());
+    for (auto operand : operands) {
----------------
nit: inline this into its use.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:237
+      else
+        return emitError(opInst.getLoc(), "Constant clauses expected");
+    }
----------------
Start diagnostics with lower case


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:228
+llvm.func @invokeLandingpad() -> !llvm.i32 {
+// CHECK-NEXT: %0 = llvm.mlir.constant(0 : i32) : !llvm.i32
+// CHECK-NEXT: %1 = llvm.mlir.constant(3 : i32) : !llvm.i32
----------------
Please do not use SSA value names directly.

https://mlir.llvm.org/getting_started/TestingGuide/

This script can help with generating check lines, but should be stripped down to something appropriate.

https://github.com/llvm/llvm-project/blob/master/mlir/utils/generate-test-checks.py


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:271
+}
\ No newline at end of file

----------------
Missing newline in some of these files.


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

https://reviews.llvm.org/D72006