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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Looking good, a few more nits.



================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:502
+
+  let assemblyFormat = [{
+    $value attr-dict `:` type($value)
----------------
nit: Can you just use a single line string? `= "...";`


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:674
+    void setPersonalityFn(FlatSymbolRefAttr personality) {
+        setAttr("personality", personality);
+    }
----------------
Can you fix the formatting here? Seems to be 2 spaces too many.


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:768
+  if (f->hasPersonalityFn()) {
+    llvm::Constant *pf = f->getPersonalityFn();
+
----------------
Can you extract this out into a new function? That should simplify a bit of the control flow here.


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:782
+                                         ->getLLVMContext())) {
+          if (auto func = dyn_cast_or_null<llvm::Function>(bc->getOperand(0)))
+            personality = b.getSymbolRefAttr(func->getName());
----------------
dyn_cast_or_null is only for when the value can be null, do we expect the operand here to ever be null? If not, use dyn_cast instead.


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:785
+        }
+        // Delete the zombie instruction
+        bc->deleteValue();
----------------
Please update each of the comments to have punctuation at the end. This applies throughout this revision.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:637
+  for (Value v : values) {
+    assert(valueMapping.find(v) != valueMapping.end() &&
+           "referencing undefined value");
----------------
nit: llvm::is_contained


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

https://reviews.llvm.org/D71888