[mlir][llvm] Verify consistency of llvm.resume and llvm.landingpad types

[victor-eds]

Summary

Following the steps of the LLVM verifier
(https://github.com/llvm/llvm-project/blob/b2c48559c882fd558f91e471c4d23ea7b0c6e718/llvm/lib/IR/Verifier.cpp#L4195),
checks in llvm.func verifier are added to ensure consistency of
llvm.landingpad operations' result types and llvm.resume operations'
input types.

As in LLVM, we will allow llvm.resume operations with input values
defined by operations other than llvm.landingpad.

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

[Dinistro]

It seems a bit wasteful to walk all the landingpads for each of the landingpads, but there might not be another way. It would be nice if one could compare the types with the result type of the personality function, but it does not exist for non-llvm functions.

What are the expected semantics of a landingpad operation that is part of something else than an llvm.func?

[victor-eds]

In D146968#4224521, @Dinistro wrote:

It seems a bit wasteful to walk all the landingpads for each of the landingpads, but there might not be another way.

Yes, I don't think there is unfortunately...

It would be nice if one could compare the types with the result type of the personality function, but it does not exist for non-llvm functions.

Also, AFAIK, this mapping is target specific (from the spec):

As with calling conventions, how the personality function results are represented in LLVM IR is target specific.

What are the expected semantics of a landingpad operation that is part of something else than an llvm.func?

There is no documentation on this operation. However, following the dialect [[documentation|https://mlir.llvm.org/docs/Dialects/LLVM/]]:

Unless explicitly stated otherwise, the semantics of the LLVM dialect operations must correspond to the semantics of LLVM IR instructions and any divergence is considered a bug.

I assume the semantics will remain unchanged regardless of the parent function operation (be it llvm.func or, e.g., func.func).

[Dinistro]

There is no documentation on this operation. However, following the dialect [[documentation|https://mlir.llvm.org/docs/Dialects/LLVM/]]:

Unless explicitly stated otherwise, the semantics of the LLVM dialect operations must correspond to the semantics of LLVM IR instructions and any divergence is considered a bug.

I'm not sure if this can directly apply to llvm dialect operations mixed into something like a func.func. Thus, one could argue that the llvm.landingpad type invariant might be violated in that case. If we do so, the type equality check should move into the llvm.func to ensure that we only walk the region once instead of N times.

Maybe @gysit has an opinion on this.

[gysit]

the type equality check should move into the llvm.func

I think moving the verifier to llvm.func sounds like a good idea. Then only a single pass is needed and there is no need to handle possible issues with non-existing parent functions. I would even consider moving the logic in https://reviews.llvm.org/D145802 to the same verifier. LLVM's verifier also works with a single pass and a global variable that is used for checking the types of landing pad and resume ops at once:

https://github.com/llvm/llvm-project/blob/15f52c1502e6aa2f7553393d76da92b21c7cf493/llvm/lib/IR/Verifier.cpp#L309

An additional advantage of the solution is that the verifier only runs if the landing pad operations are part of an llvm.func. If not, for example, during testing or while the IR is in a transient state in between lowering passes the verifier does not run. This behavior is in line with the existing verifier that only checks for the personality function if the landing pad op is part of a llvm.func.

[victor-eds]

Check type consistency in llvm.func verifier

[victor-eds]

In D146968#4226524, @gysit wrote:

the type equality check should move into the llvm.func

I think moving the verifier to llvm.func sounds like a good idea. Then only a single pass is needed and there is no need to handle possible issues with non-existing parent functions. I would even consider moving the logic in https://reviews.llvm.org/D145802 to the same verifier. LLVM's verifier also works with a single pass and a global variable that is used for checking the types of landing pad and resume ops at once:

https://github.com/llvm/llvm-project/blob/15f52c1502e6aa2f7553393d76da92b21c7cf493/llvm/lib/IR/Verifier.cpp#L309

An additional advantage of the solution is that the verifier only runs if the landing pad operations are part of an llvm.func. If not, for example, during testing or while the IR is in a transient state in between lowering passes the verifier does not run. This behavior is in line with the existing verifier that only checks for the personality function if the landing pad op is part of a llvm.func.

I agree we can move this logic to the llvm.func verifier if the semantics are not to be preserved in other function operations. Now this patch mimics LLVM behavior.

[zero9178]

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

2179

nit: please just use capture-all & for the lambdas here

2179

Could you extract the walk call out of the if condition? I think it makes the logic harder to see and increases the indentation within walk.

2193

No need to specify the type here. Case deduces it from the lambda parameter

2199

[gysit]

Thanks for the changes LGTM modulo @zero9178 's comments.

mlir/test/Dialect/LLVMIR/invalid-typed-pointers.mlir

141

ultra nit: there is now a expected-error@below for matching errors on the next line.

Same holds for the tests below.

[Dinistro]

+1 on zero's comments. Once they are implemented: LGTM!

[victor-eds]

Address comments

[victor-eds]

Rename var

[victor-eds]

Improve readability

[victor-eds]

Thanks for the comments everyone! I'll land this now.

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

 LogicalResult LandingpadOp::verify() {
   Value value;
   if (LLVMFuncOp func = (*this)->getParentOfType<LLVMFuncOp>()) {
     if (!func.getPersonality())
       return emitError(
           "llvm.landingpad needs to be in a function with a personality");
   }
 
+  // Consistency of llvm.landingpad result types is checked in
+  // LLVMFuncOp::verify().
+
   if (!getCleanup() && getOperands().empty())
     return emitError("landingpad instruction expects at least one clause or "
                      "cleanup attribute");
 
   for (unsigned idx = 0, ie = getNumOperands(); idx < ie; idx++) {
     value = getOperand(idx);
     bool isFilter = value.getType().isa<LLVMArrayType>();
     if (isFilter) {
LogicalResult ReturnOp::verify() {
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // ResumeOp
 //===----------------------------------------------------------------------===//
 
 LogicalResult ResumeOp::verify() {
-  if (!getValue().getDefiningOp<LandingpadOp>())
-    return emitOpError("expects landingpad value as operand");
+  // Consistency of llvm.resume value types is checked in LLVMFuncOp::verify().
+
   // No check for personality of function - landingpad op verifies it.
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Verifier for LLVM::AddressOfOp.
 //===----------------------------------------------------------------------===//
 
if (getLinkage() != LLVM::Linkage::External &&
       return emitOpError() << "external functions must have '"
                            << stringifyLinkage(LLVM::Linkage::External)
                            << "' or '"
                            << stringifyLinkage(LLVM::Linkage::ExternWeak)
                            << "' linkage";
     return success();
   }
 
+  Type landingpadResultTy;
+  StringRef diagnosticMessage;
+  bool isLandingpadTypeConsistent =
+      !walk([&](Operation *op) {
+         const auto checkType = [&](Type type, StringRef errorMessage) {
+           if (!landingpadResultTy) {
+             landingpadResultTy = type;
+             return WalkResult::advance();
+           }
+           if (landingpadResultTy != type) {
+             diagnosticMessage = errorMessage;
+             return WalkResult::interrupt();
+           }
+           return WalkResult::advance();
+         };
+         return TypeSwitch<Operation *, WalkResult>(op)
+             .Case<LandingpadOp>([&](auto landingpad) {
+               constexpr StringLiteral errorMessage =
+                   "'llvm.landingpad' should have a consistent result type "
+                   "inside a function";
+               return checkType(landingpad.getType(), errorMessage);
+             })
+             .Case<ResumeOp>([&](auto resume) {
+               constexpr StringLiteral errorMessage =
+                   "'llvm.resume' should have a consistent input type inside a "
+                   "function";
+               return checkType(resume.getValue().getType(), errorMessage);
+             })
+             .Default([](auto) { return WalkResult::skip(); });
+       }).wasInterrupted();
+  if (!isLandingpadTypeConsistent) {
+    assert(!diagnosticMessage.empty() &&
+           "Expecting a non-empty diagnostic message");
+    return emitError(diagnosticMessage);
+  }
+
   return success();
 }
 
 /// Verifies LLVM- and implementation-specific properties of the LLVM func Op:
 /// - entry block arguments are of LLVM types.
 LogicalResult LLVMFuncOp::verifyRegions() {
   if (isExternal())
     return success();

mlir/test/Dialect/LLVMIR/invalid-typed-pointers.mlir

^bb2: // pred: ^bb0
   llvm.return %0 : i32
 }
 
 // -----
 
 llvm.func @foo(i32) -> i32
 llvm.func @__gxx_personality_v0(...) -> i32
 
+// expected-error@below {{'llvm.resume' should have a consistent input type inside a function}}
+llvm.func @caller(%arg0: i32, %arg1: !llvm.struct<(ptr<i32>, i32)>) -> i32 attributes { personality = @__gxx_personality_v0 } {
+  %0 = llvm.invoke @foo(%arg0) to ^bb1 unwind ^bb2 : (i32) -> i32
+^bb1:
+  %1 = llvm.invoke @foo(%0) to ^bb3 unwind ^bb4 : (i32) -> i32
+^bb2:
+  %2 = llvm.landingpad cleanup : !llvm.struct<(ptr<i8>, i32)>
+  llvm.resume %arg1 : !llvm.struct<(ptr<i32>, i32)>
+^bb3:
+  llvm.return %1 : i32
+^bb4:
+  %3 = llvm.landingpad cleanup : !llvm.struct<(ptr<i8>, i32)>
+  llvm.resume %3 : !llvm.struct<(ptr<i8>, i32)>
+}
+
+// -----
+
+llvm.func @foo(i32) -> i32
+llvm.func @__gxx_personality_v0(...) -> i32
+
+// expected-error@below {{'llvm.landingpad' should have a consistent result type inside a function}}
 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
+  %0 = llvm.invoke @foo(%arg0) to ^bb1 unwind ^bb2 : (i32) -> i32
+^bb1:
+  %1 = llvm.invoke @foo(%0) to ^bb3 unwind ^bb4 : (i32) -> i32
+^bb2:
   %2 = llvm.landingpad cleanup : !llvm.struct<(ptr<i8>, i32)>
-  // expected-error@+1 {{'llvm.resume' op expects landingpad value as operand}}
-  llvm.resume %0 : i32
+  llvm.resume %2 : !llvm.struct<(ptr<i8>, i32)>
+^bb3:
+  llvm.return %1 : i32
+^bb4:
+  %3 = llvm.landingpad cleanup : !llvm.struct<(ptr<i32>, i32)>
+  llvm.resume %3 : !llvm.struct<(ptr<i32>, i32)>
 }
 
 // -----
 
 llvm.func @foo(i32) -> i32
 
 llvm.func @caller(%arg0: i32) -> i32 {
   %0 = llvm.mlir.constant(1 : i32) : i32

mlir/test/Dialect/LLVMIR/invalid.mlir

^bb2: // pred: ^bb0
   llvm.return %0 : i32
 }
 
 // -----
 
 llvm.func @foo(i32) -> i32
 llvm.func @__gxx_personality_v0(...) -> i32
 
+// expected-error@below {{'llvm.resume' should have a consistent input type inside a function}}
 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
+  %0 = llvm.invoke @foo(%arg0) to ^bb1 unwind ^bb2 : (i32) -> i32
+^bb1:
+  %1 = llvm.invoke @foo(%0) to ^bb3 unwind ^bb4 : (i32) -> i32
+^bb2:
   %2 = llvm.landingpad cleanup : !llvm.struct<(ptr, i32)>
-  // expected-error@+1 {{'llvm.resume' op expects landingpad value as operand}}
   llvm.resume %0 : i32
+^bb3:
+  llvm.return %1 : i32
+^bb4:
+  %3 = llvm.landingpad cleanup : !llvm.struct<(ptr, i32)>
+  llvm.resume %3 : !llvm.struct<(ptr, i32)>
+}
+
+// -----
+
+llvm.func @foo(i32) -> i32
+llvm.func @__gxx_personality_v0(...) -> i32
+
+// expected-error@below {{'llvm.landingpad' should have a consistent result type inside a function}}
+llvm.func @caller(%arg0: i32) -> i32 attributes { personality = @__gxx_personality_v0 } {
+  %0 = llvm.invoke @foo(%arg0) to ^bb1 unwind ^bb2 : (i32) -> i32
+^bb1:
+  %1 = llvm.invoke @foo(%0) to ^bb3 unwind ^bb4 : (i32) -> i32
+^bb2:
+  %2 = llvm.landingpad cleanup : !llvm.struct<(ptr, i32)>
+  llvm.resume %2 : !llvm.struct<(ptr, i32)>
+^bb3:
+  llvm.return %1 : i32
+^bb4:
+  %3 = llvm.landingpad cleanup : i32
+  llvm.resume %3 : i32
 }
 
 // -----
 
 llvm.func @foo(i32) -> i32
 
 llvm.func @caller(%arg0: i32) -> i32 {
   %0 = llvm.mlir.constant(1 : i32) : i32