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

[Shraiysh via Phabricator via llvm-commits]

shraiysh added a comment.

Addressed comments.



================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:777
+        // Delete the zombie instruction.
+        bc->deleteValue();
+      }
----------------
ftynse wrote:
> Why is this necessary? I wouldn't expect the converter to modify the input module silently.
If this is not done, the conversion crashes with the error `Use still stuck around after Def is destroyed`.


================
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);
----------------
ftynse wrote:
> Do we always want to bitcast symbol references? E.g., what happens for good old function constants that have the right type?
When the type is same, the `bitcast` instruction is automatically omitted in this behaviour (must be handled somewhere else). I tried running it on a function pointer for which both the types were same and it did not produce a new bitcast instruction while converting to LLVM IR.


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

https://reviews.llvm.org/D71888