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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

In D71888#1853128 <https://reviews.llvm.org/D71888#1853128>, @shraiysh wrote:

> Please review this.
>
> Also, I am unable to figure out how to model constant expressions for personality in MLIR.
>
> For example,
>
>   define dso_local i32 @main() personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
>     ...
>   }
>
>
> will break right now, because I could not model `bitcast` globally.
>  Any insights into how do I get bitcast to work as a global instruction will help.
> Thanks


This is a bit thorny. I'm not entirely sure on the contract of how the personality function is supposed to look. I went back through the commits in clang, but couldn't find anything that definitively documented it. I would say for now we could likely just always bitcast functions to an opaque i8* when exporting. That would mean that our roundtrip isn't exactly exact lossless, but that is a separate issue. Also, I have seen usages of i32 values as the personality function in some tests before. I'm not sure if any particular front-end uses anything other than a function pointer, but it is something to keep in mind. I'm fine with keeping the personality function as a FlatSymbolRef for now though.



================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:487
+  let verifier = [{
+    if(getNumOperands() != 1)
+      return emitOpError("expects 1 operand");
----------------
nit: Can you override the arguments of this op so that this is handled automatically?


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:931
+
+// <operation> ::= `llvm.resume` ssa-use-list attribute-dict? `:`
+//                 type-list-no-parens
----------------
nit: Can you use the declarative custom assembly form instead? That would remove the need to define the parser/printer in c++.

https://mlir.llvm.org/docs/OpDefinitions/#declarative-assembly-format

Should be something like:

```
let assemblyFormat = "operands attr-dict `:` type(operands)";
```


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:700
+
+    Operation *op = b.create<LandingpadOp>(loc, ty, lpi->isCleanup(), ops);
+    v = op->getResult(0);
----------------
nit: `v = b.create<LandingPadOp>(...);`


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:775
+    if (personality) {
+      fop = b.create<LLVMFuncOp>(UnknownLoc::get(context), f->getName(),
+                                 functionType, personality);
----------------
nit: Can you create the function and then set the personality afterwards? That seems like it would be much cleaner.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:547
+  if (func.personality().hasValue())
+    llvmFunc->setPersonalityFn(
+        functionMapping.lookup(func.personality().getValue()));
----------------
Can you just use the normal attribute export handling here?


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:612
+  for (Value v : values) {
+    assert(valueMapping.find(v) != valueMapping.end() &&
+           "referencing undefined value");
----------------
What prompted this change? Seems unrelated.


================
Comment at: mlir/test/Dialect/LLVMIR/invalid.mlir:611
+  llvm.resume %2 : !llvm<"{ i8*, i32 }">
+}
----------------
Missing newline after this.


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

https://reviews.llvm.org/D71888