[mlir][llvm] Allow llvm.resume with non-landingpad input

[victor-eds]

Summary

This changes verification of llvm.resume to just fail if there is no
llvm.landingpad instruction in the same function with the same type as
this instruction's input, more in line with LLVM's specification. From
LLVM documentation (https://llvm.org/docs/LangRef.html#i-resume):

The ‘resume’ instruction requires one argument, which must have the same
type as the result of any ‘landingpad’ instruction in the same function.

Signed-off-by: Victor Perez <[e-mail]>

[victor-eds]

Ping

[zero9178]

Looks good to me with some notes:

• LLVMs verifier seems to work differently by checking that ALL landing pads and resume instructions use consistent types: https://github.com/llvm/llvm-project/blob/b2c48559c882fd558f91e471c4d23ea7b0c6e718/llvm/lib/IR/Verifier.cpp#L4196 This seems contrary to what the langref says however which is confusing to me.
• Could you rebase the patch? Recent changes have landed that has turned all these tests to opaque pointers.

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

1528

nit: Use Type here instead of const auto

[victor-eds]

Use Type instead of const auto and use opaque pointers in tests.

[victor-eds]

In D145802#4219686, @zero9178 wrote:

• LLVMs verifier seems to work differently by checking that ALL landing pads and resume instructions use consistent types: https://github.com/llvm/llvm-project/blob/b2c48559c882fd558f91e471c4d23ea7b0c6e718/llvm/lib/IR/Verifier.cpp#L4196 This seems contrary to what the langref says however which is confusing to me.

Yes, I saw that the code differs from the spec. According to the specification of the landingpad instruction:

The ‘landingpad’ instruction defines the values which are set by the personality function upon re-entry to the function, and therefore the “result type” of the landingpad instruction. As with calling conventions, how the personality function results are represented in LLVM IR is target specific.

I'd argue it would make sense to check every llvm.landingpad operation in a function has the same type (not sure if we should check it in llvm.resume's verifier). If you don't mind, I can address that in a different patch to keep this one small and simple and separate concerns.

[zero9178]

In D145802#4223741, @victor-eds wrote:

In D145802#4219686, @zero9178 wrote:

• LLVMs verifier seems to work differently by checking that ALL landing pads and resume instructions use consistent types: https://github.com/llvm/llvm-project/blob/b2c48559c882fd558f91e471c4d23ea7b0c6e718/llvm/lib/IR/Verifier.cpp#L4196 This seems contrary to what the langref says however which is confusing to me.

Yes, I saw that the code differs from the spec. According to the specification of the landingpad instruction:

The ‘landingpad’ instruction defines the values which are set by the personality function upon re-entry to the function, and therefore the “result type” of the landingpad instruction. As with calling conventions, how the personality function results are represented in LLVM IR is target specific.

I'd argue it would make sense to check every llvm.landingpad operation in a function has the same type (not sure if we should check it in llvm.resume's verifier). If you don't mind, I can address that in a different patch to keep this one small and simple and separate concerns.

I think it is fine as is. Making the verifier not have false positives is much more important than catching false negatives.

LGTM, but please wait a day or two for others to check as well. Ading some extra reviewers for visibility as well.

[victor-eds]

Assume all llvm.landingpad operations have the same type.

[victor-eds]

In D145802#4223771, @zero9178 wrote:

In D145802#4223741, @victor-eds wrote:

In D145802#4219686, @zero9178 wrote:

• LLVMs verifier seems to work differently by checking that ALL landing pads and resume instructions use consistent types: https://github.com/llvm/llvm-project/blob/b2c48559c882fd558f91e471c4d23ea7b0c6e718/llvm/lib/IR/Verifier.cpp#L4196 This seems contrary to what the langref says however which is confusing to me.

Yes, I saw that the code differs from the spec. According to the specification of the landingpad instruction:

The ‘landingpad’ instruction defines the values which are set by the personality function upon re-entry to the function, and therefore the “result type” of the landingpad instruction. As with calling conventions, how the personality function results are represented in LLVM IR is target specific.

I'd argue it would make sense to check every llvm.landingpad operation in a function has the same type (not sure if we should check it in llvm.resume's verifier). If you don't mind, I can address that in a different patch to keep this one small and simple and separate concerns.

I think it is fine as is. Making the verifier not have false positives is much more important than catching false negatives.

LGTM, but please wait a day or two for others to check as well. Ading some extra reviewers for visibility as well.

Yes, I'll wait till Wednesday morning (European time) to land. I've modified the code so that we assume all landingpad operations have the same type, as they should.

[victor-eds]

Allow nested ops

[victor-eds]

Drop include

[victor-eds]

Rebase

[victor-eds]

Run tests

[gysit]

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

1527

I would probably return failure in this case since this can theoretically happen if the operation is used in some custom function type that does not implement FunctionOpInterface?

1529

If I understand the discussion correctly then we want to test that the type of all landing pad instructions in the function match the type of the resume operand type. Wouldn't that require adding all types to a vector and then checking all of them match? The current code seems to stop after matching the first landingpad op?

[victor-eds]

Address review comments

[gysit]

Thanks, LGTM!

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

1529

Got it, the assumption is there is another verifier that checks all the types are the same...

1530

nit: let's drop the braces here and in for the if below since the body contains a single statement.

[victor-eds]

Add test

[victor-eds]

Drop braces

[victor-eds]

I addressed your comments already

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

1527

I did that and added a test.

1529

If we follow the spec closely, it'd make sense to simply check there is an llvm.landingpad operation in the function with the same type as the input type of the current operation.

As all llvm.landingpad operations should have the same type (according to the LLVM spec and its verifier), it'd be enough to check a single operation here.

I'm working on a patch checking all llvm.landingpad operations have the same type (we could land that one before this one)

[gysit]

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

1529

Yes perfect! That's why I accepted the revision :). The "got it" comment was meant as an explanation for my earlier comment. Sorry for the confusion.

[victor-eds]

Moved to https://reviews.llvm.org/D146968