This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Added llvm.resume and personality functions in LLVM IR Dialect
ClosedPublic

Authored by shraiysh on Dec 26 2019, 1:24 AM.

Details

Summary

llvm.resume is similar to llvm.return except that has to be exactly one operand and that should be derived from a llvm.landingpad instruction.
Any function having llvm.landingpad instruction must have a personality attribute.

Example:
LLVM IR

define dso_local i32 @main() personality i32 (...)* @__gxx_personality_v0 {
  invoke void @foo(i32 42)
          to label %3 unwind label %1

1:                                                ; preds = %0
  %2 = landingpad i8*
          catch i8** @_ZTIi
          catch i8* bitcast (i8** @_ZTIi to i8*)
  resume i8* %2

3:                                                ; preds = %0
  ret i32 1
}

MLIR - LLVM IR Dialect

llvm.func @main() -> !llvm.i32 attributes {personality = @__gxx_personality_v0} {
    %0 = llvm.mlir.constant(1 : i32) : !llvm.i32
    %1 = llvm.mlir.addressof @_ZTIi : !llvm<"i8**">
    %2 = llvm.bitcast %1 : !llvm<"i8**"> to !llvm<"i8*">
    %3 = llvm.mlir.addressof @_ZTIi : !llvm<"i8**">
    %4 = llvm.mlir.constant(42 : i32) : !llvm.i32
    llvm.invoke @foo(%4) to ^bb2 unwind ^bb1 : (!llvm.i32) -> ()
  ^bb1:	// pred: ^bb0
    %5 = llvm.landingpad (catch %3 : !llvm<"i8**">) (catch %2 : !llvm<"i8*">) : !llvm<"i8*">
    llvm.resume %5 : !llvm<"i8*">
  ^bb2:	// pred: ^bb0
    llvm.return %0 : !llvm.i32
  }

Diff Detail

Event Timeline

shraiysh created this revision.Dec 26 2019, 1:24 AM
shraiysh updated this revision to Diff 235329.EditedDec 26 2019, 1:45 AM

Looks like landingpad needs to be implemented before doing this. Will come back to this after that is done.

ftynse added a subscriber: ftynse.Dec 26 2019, 6:37 AM

Thanks for working on this! Please feel free to add me as a reviewer when you are have landingpad implemented.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
511

The number of operands should be checked by declaring them in the "arguments" list with appropriate types. This will need another check to make sure the type matches that mentioned in some "landingpad", but that check can assume the "arguments"-based check passed.

Note that in MLIR, operation verifiers are not allowed to inspect sibling operations (since operations can be processed in parallel). So the type-match verifier should be implemented on the enclosing operation, likely in LLVMFuncOp.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
862

This should be enforced by the verifier. Generally, we don't check the verifier-enforced properties in functions.

872

If there's strictly one operand, I'd suggest using parser.parseOperand instead. It would provide a more meaningful message in case the operand is absent.

rriddle added inline comments.Dec 26 2019, 9:22 AM
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
861

ResumeOp &

Operations are value-typed, so you shouldn't pass by reference or pointer in general.

mehdi_amini added inline comments.Dec 26 2019, 10:38 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
511

Note that in MLIR, operation verifiers are not allowed to inspect sibling operations (since operations can be processed in parallel).

To clarify: this is only true for symbolic reference region-traversal: it is valid to traverse use-def chains in a verifier I believe.

Thank you all for your comments. I will get back to this issue after doing the landingpad instruction.

shraiysh updated this revision to Diff 241873.Feb 1 2020, 3:04 AM
shraiysh retitled this revision from [mlir] Added llvm.resume in LLVM IR Dialect to [mlir] Added llvm.resume and personality functions in LLVM IR Dialect.
shraiysh edited the summary of this revision. (Show Details)
shraiysh added a reviewer: rriddle.

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

Hi, can someone please provide a review for this patch.
Thanks.

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.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
487

nit: Can you override the arguments of this op so that this is handled automatically?

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
931

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)";
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
700

nit: v = b.create<LandingPadOp>(...);

775

nit: Can you create the function and then set the personality afterwards? That seems like it would be much cleaner.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
547

Can you just use the normal attribute export handling here?

612

What prompted this change? Seems unrelated.

mlir/test/Dialect/LLVMIR/invalid.mlir
611

Missing newline after this.

Hi, can someone please provide a review for this patch.
Thanks.

Sorry for the delay.

rriddle requested changes to this revision.Feb 6 2020, 8:20 PM
This revision now requires changes to proceed.Feb 6 2020, 8:20 PM
shraiysh updated this revision to Diff 244323.Feb 12 2020, 7:25 PM
shraiysh marked 7 inline comments as done.
shraiysh added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
612

In landingpad instruction, valuMapping was not set. Hence, while trying to make it an operand in resume, the program was going into a segmentation fault. I wrote this assert to avoid the segmentation fault.

Looking good, a few more nits.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
502

nit: Can you just use a single line string? = "...";

674

Can you fix the formatting here? Seems to be 2 spaces too many.

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
768

Can you extract this out into a new function? That should simplify a bit of the control flow here.

782

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.

785

Please update each of the comments to have punctuation at the end. This applies throughout this revision.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
637

nit: llvm::is_contained

shraiysh updated this revision to Diff 244955.Feb 17 2020, 6:01 AM
shraiysh marked 6 inline comments as done.
shraiysh added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
637

I think llvm::is_contained is a wrapper for std::find. I am not sure if there is a neat way of looking for keys in a map using std::find (and hence llvm::is_contained). Please let me know if there is.

bondhugula added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
637

It might be a wrapper around it, but it's more readable to me with llvm::is_contained.

shraiysh marked an inline comment as done.Feb 17 2020, 8:57 AM
shraiysh added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
637

I meant llvm::is_contained(valueMapping, v) does not work - because valueMapping is a DenseMap. I am not aware of a variant of is_contained that supports maps - looking for a key in it. Currently I am using std::map::find while the documentation suggests is_contained is wrapper for std::find.

mehdi_amini added inline comments.Feb 17 2020, 5:59 PM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
637

rather valueMapping.count(v) ?

rriddle added inline comments.Feb 17 2020, 7:30 PM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
637

Gah, I was hoping that llvm::is_contained would be smart enough to detect find at this point. Given that, you should use .count() instead.

shraiysh updated this revision to Diff 245137.Feb 18 2020, 5:47 AM
shraiysh marked 5 inline comments as done.
shraiysh added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
637

Done!

I do not have write access to the repository. If would be great if someone could commit for me. Thanks.

shraiysh updated this revision to Diff 246057.Feb 21 2020, 10:05 PM

Updated with full context.

ftynse requested changes to this revision.Feb 24 2020, 6:04 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
513

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 (.

692

Don't we have a tablegenrated equivalent?

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
809

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. */
821–822

LLVMDialect is cached by the Importer, so just dialect->getLLVMContext() should be sufficient

828

Why is this necessary? I wouldn't expect the converter to modify the input module silently.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
102–103

Do we always want to bitcast symbol references? E.g., what happens for good old function constants that have the right type?

mlir/test/Dialect/LLVMIR/invalid.mlir
577

You don't need to test the default parser.

600

This line doesn't seem necessary in this test

This revision now requires changes to proceed.Feb 24 2020, 6:04 AM
shraiysh updated this revision to Diff 247171.Feb 28 2020, 12:04 AM
shraiysh marked 10 inline comments as done.

Addressed comments.

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
828

If this is not done, the conversion crashes with the error Use still stuck around after Def is destroyed.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
102–103

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.

shraiysh added a comment.EditedMar 9 2020, 4:28 AM

@ftynse @rriddle please review this

rriddle added inline comments.Mar 10 2020, 10:41 AM
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
63

nit: -> /

63

(sigh, looks like the display is messed up)
nit: Use /// for top-level comments.

820

We shouldn't materialize an instruction here, you can just use ce->getOpCode() == llvm::Instruction::BitCast.

shraiysh updated this revision to Diff 249626.EditedMar 11 2020, 7:54 AM
shraiysh marked 3 inline comments as done.

Addressed comments.

uenoku added a subscriber: uenoku.Mar 12 2020, 10:30 AM
rriddle accepted this revision.Mar 16 2020, 9:52 AM
rriddle added inline comments.
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
816

Can we change these to early return instead?

if (...)

return b...;

if (auto ce ...) {

}
return FlatSymbolRefAttr();

ftynse accepted this revision.Mar 18 2020, 2:40 AM
This revision is now accepted and ready to land.Mar 18 2020, 2:40 AM
shraiysh updated this revision to Diff 251343.Mar 19 2020, 4:42 AM
shraiysh marked an inline comment as done.

Addressed comments.

I do not have commit access. Can someone please commit this?
Thanks

This revision was automatically updated to reflect the committed changes.