[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

1527

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

1526

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?

1528

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

1528

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

1529

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

1526

I did that and added a test.

1528

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

1528

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

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

LogicalResult ReturnOp::verify() {
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // ResumeOp
 //===----------------------------------------------------------------------===//
 
 LogicalResult ResumeOp::verify() {
-  if (!getValue().getDefiningOp<LandingpadOp>())
-    return emitOpError("expects landingpad value as operand");
+  auto parentFunc = getOperation()->getParentOfType<FunctionOpInterface>();
+  assert(parentFunc && "Expecting parent function");
+  const auto ty = getOperand().getType();
+  if (!parentFunc
+           ->walk([ty](LandingpadOp landingpad) {
+             return landingpad.getType() == ty
+                        ? WalkResult::interrupt() // Just an operation needed
+                        : WalkResult::advance();
+           })
+           .wasInterrupted()) {
+    // No operation was found: emit error
+    return emitOpError("expects landingpad operation in the same function and "
+                       "with the same type as this operation's operand");
+  }
+
   // No check for personality of function - landingpad op verifies it.
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Verifier for LLVM::AddressOfOp.
 //===----------------------------------------------------------------------===//
 

mlir/test/Dialect/LLVMIR/invalid.mlir

 
 llvm.func @caller(%arg0: i32) -> i32 attributes { personality = @__gxx_personality_v0 } {
   %0 = llvm.mlir.constant(1 : i32) : i32
   %1 = llvm.invoke @foo(%0) to ^bb1 unwind ^bb2 : (i32) -> i32
 ^bb1: // pred: ^bb0
   llvm.return %0 : i32
 ^bb2: // pred: ^bb0
   %2 = llvm.landingpad cleanup : !llvm.struct<(ptr<i8>, i32)>
-  // expected-error@+1 {{'llvm.resume' op expects landingpad value as operand}}
+  // expected-error@+1 {{'llvm.resume' op expects landingpad operation in the same function and with the same type as this operation's operand}}
+  llvm.resume %0 : i32
+}
+
+// -----
+
+llvm.func @foo(i32) -> i32
+llvm.func @__gxx_personality_v0(...) -> i32
+
+llvm.func @caller(%arg0: i32) -> i32 attributes { personality = @__gxx_personality_v0 } {
+  %0 = llvm.mlir.constant(1 : i32) : i32
+  // expected-error@+1 {{'llvm.resume' op expects landingpad operation in the same function and with the same type as this operation's operand}}
   llvm.resume %0 : i32
 }
 
 // -----
 
 llvm.func @foo(i32) -> i32
 
 llvm.func @caller(%arg0: i32) -> i32 {

mlir/test/Dialect/LLVMIR/roundtrip.mlir

^bb4:
   %13 = llvm.invoke %12(%7) to ^bb2 unwind ^bb1 : !llvm.ptr, (i32) -> !llvm.struct<(i32, f64, i32)>
 
 // CHECK: ^[[BB5:.*]]:
 // CHECK:   llvm.return %[[a0]] : i32
 ^bb5:
   llvm.return %0 : i32
 }
 
+llvm.func @foo2() -> !llvm.struct<(ptr<i8>, i32)>
+
+// CHECK-LABEL: llvm.func @resumeNoLandingpadValue(
+// CHECK-SAME:                                     %[[ARG:.*]]: i32)
+llvm.func @resumeNoLandingpadValue(%arg0: i32) -> i32 attributes { personality = @__gxx_personality_v0 } {
+// CHECK: %[[VAL_0:.*]] = llvm.call @foo2() : () -> !llvm.struct<(ptr<i8>, i32)>
+// CHECK: %[[VAL_1:.*]] = llvm.invoke @foo(%[[ARG]]) to ^[[BB1:.*]] unwind ^[[BB2:.*]] : (i32) -> i32
+  %0 = llvm.call @foo2() : () -> !llvm.struct<(ptr<i8>, i32)>
+  %1 = llvm.invoke @foo(%arg0) to ^bb1 unwind ^bb2 : (i32) -> i32
+// CHECK: ^[[BB1]]:
+// CHECK: llvm.return %[[VAL_1]] : i32
+^bb1: // pred: ^bb0
+  llvm.return %1 : i32
+// CHECK: ^[[BB2]]:
+// CHECK: %[[VAL_2:.*]] = llvm.landingpad cleanup : !llvm.struct<(ptr<i8>, i32)>
+// CHECK: llvm.resume %[[VAL_0]] : !llvm.struct<(ptr<i8>, i32)>
+^bb2: // pred: ^bb0
+  %2 = llvm.landingpad cleanup : !llvm.struct<(ptr<i8>, i32)>
+  llvm.resume %0 : !llvm.struct<(ptr<i8>, i32)>
+}
+
 // CHECK-LABEL: @useFreezeOp
 func.func @useFreezeOp(%arg0: i32) {
   // CHECK:  = llvm.freeze %[[ARG0:.*]] : i32
   %0 = llvm.freeze %arg0 : i32
   // CHECK: %[[x:.*]] = llvm.mlir.undef : i8
   %1 = llvm.mlir.undef : i8
   // CHECK:  = llvm.freeze %[[x]] : i8
   %2 = llvm.freeze %1 : i8