[PATCH] D71888: [mlir] Added llvm.resume and personality functions in LLVM IR Dialect

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:496
+  let verifier = [{
+    if(!isa<LandingpadOp>(value().getDefiningOp()))
+      return emitOpError("expects landingpad value as operand");
----------------
Value is not guaranteed to have a defining op, use `isa_and_nonnull` instead to avoid a crash here.

Nit: add a space between `if` and `(`. 


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:671
     }
+    void setPersonalityFn(FlatSymbolRefAttr personality) {
+      setAttr("personality", personality);
----------------
Don't we have a tablegenrated equivalent?


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:758
+  FlatSymbolRefAttr personality;
+  if (f->hasPersonalityFn()) {
+    llvm::Constant *pf = f->getPersonalityFn();
----------------
LLVM prefers early-return over long control flow blocks, e.g.

```
if (!f->hasPersinalityFn())
  return nullptr;

FlatSymbolRefAttr personality;
/* The code you have inside if, but with less indentation. */
```


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:770-771
+        if (bc->getDestTy() ==
+            llvm::Type::getInt8PtrTy(b.getContext()
+                                         ->getRegisteredDialect<LLVMDialect>()
+                                         ->getLLVMContext())) {
----------------
LLVMDialect is cached by the Importer, so just `dialect->getLLVMContext()` should be sufficient


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:777
+        // Delete the zombie instruction.
+        bc->deleteValue();
+      }
----------------
Why is this necessary? I wouldn't expect the converter to modify the input module silently.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:98
   if (auto funcAttr = attr.dyn_cast<FlatSymbolRefAttr>())
-    return functionMapping.lookup(funcAttr.getValue());
+    return llvm::ConstantExpr::getBitCast(
+        functionMapping.lookup(funcAttr.getValue()), llvmType);
----------------
Do we always want to bitcast symbol references? E.g., what happens for good old function constants that have the right type?


================
Comment at: mlir/test/Dialect/LLVMIR/invalid.mlir:577
+  %2 = llvm.landingpad cleanup : !llvm<"{ i8*, i32 }">
+  // expected-error at +1 {{expected SSA operand}}
+  llvm.resume : !llvm<"{ i8*, i32 }">
----------------
You don't need to test the default parser.


================
Comment at: mlir/test/Dialect/LLVMIR/invalid.mlir:600
+llvm.func @foo(!llvm.i32) -> !llvm.i32
+llvm.func @__gxx_personality_v0(...) -> !llvm.i32
+
----------------
This line doesn't seem necessary in this test


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

https://reviews.llvm.org/D71888