[PATCH] D72006: [MLIR] Added llvm.invoke and llvm.landingpad

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.
Herald added a subscriber: nicolasvasilache.

Haven't looked at the code, let met know if you want me to. Please see design comments inline.

> I have tried to implement llvm.invoke and llvm.landingpad.
> 
> llvm.invoke is similar to llvm.call with two successors added, the first one is the normal label and the second one is unwind label.
>  llvm.call was breaking for function pointers inside code, so I did not handle the same for llvm.invoke

Breaking how? If you think there's a bug, please report it (and feel free to assign to me).

> handling that can probably be a separate commit as it is independent on this. Both call and invoke might need revision after that.

I have a slight preference of not changing the IR often, i.e. if we know `invoke` will be modeled differently soon, why not model it differently from the start?

> llvm.launchpad takes a variable number of args with either catch or filter associated with them. I went with a string attribute dictionary for a list of catch and filter.

This could work, but there may be another way. Since the clauses must refer to globals anyway, we can have a list of attributes where the i-th attribute is a SymbolRef to the global if it is a catch clause, and a nested potentially empty array of SymbolRefs to globals if it is a filter clause. This is a more intrusive change compared to LLVM IR, but it's justifiable IMO since we model globals differently anyway. We can optionally store the array types as other attributes but they can always be inferred from array attributes. I haven't thought deeply about the trade-offs involved.

This would give something like
`"llvm.landingpad"() {[@_ZTli, [], @_ZTd, [@_ZTf]} : () -> !llvm<"result_type">`
in the generic form. It will remove the need to do `addressof` and declare constants before the `landingpad` (simplifying the verifier that checks whether it's the first operation in the block, and another one that checks the operands are only constants), but will make it hard to see the types.

> Not sure if implementing another type constraint alongside LLVM_Type would be a better idea.

I don't think type constraints are relevant here. You need to _store_ the fact that an operand belongs to one clause or another, constraints are only checked in the verifier and don't affect the storage.

> llvm.launchpad expects an array as an argument when filtering. Constant array and Undefined values were not handled and the program was going into a segmentation fault.

Please report a bug about the segmentation fault.

> I could test that it should work using i8 constants, which are interpreted as strings, but this is not rigorous testing.
> Examples:
>  LLVM IR
> 
> define i32 @caller(i32 %a) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
> 
>     invoke i32 @foo(i32 2) to label %success unwind label %fail
>    
>   success:
>     ret i32 2
>    
>   fail:
>     landingpad {i8*, i32} catch i8** null catch i8* bitcast (i8** @_ZTIi to i8*) filter [1 x i8] [ i8 1 ]
>     ret i32 3
> 
> }
>  MLIR LLVM Dialect
> 
> llvm.func @caller(%arg0: !llvm.i32) -> !llvm.i32 {
> 
>     %0 = llvm.mlir.constant(3 : i32) : !llvm.i32
>     %1 = llvm.mlir.constant("\01") : !llvm<"[1 x i8]">
>     %2 = llvm.mlir.addressof @_ZTIi : !llvm<"i8**">
>     %3 = llvm.bitcast %2 : !llvm<"i8**"> to !llvm<"i8*">
>     %4 = llvm.mlir.null : !llvm<"i8**">
>     %5 = llvm.mlir.constant(2 : i32) : !llvm.i32
>     %6 = llvm.invoke @foo(%5) ^bb1, ^bb2 : (!llvm.i32) -> !llvm.i32
>   ^bb1:	// pred: ^bb0
>     llvm.return %5 : !llvm.i32
>   ^bb2:	// pred: ^bb0
>     %7 = llvm.landingpad  {clauseTypes = ["catch", "catch", "filter"], cleanup = false} %4, %3, %1 : !llvm<"{ i8*, i32 }">
>     llvm.return %0 : !llvm.i32
>   }
> 
> Is this syntax okay? Should go ahead with verifiers and tests for currently handled inputs?

I think you can even have the llvm-like syntax:
`llvm.landingpad catch %4, catch %3, filter %1 : !llvm<"{ i8*, i32 }">`

You can do something like

  StringRef kw;
  while (succeeded(parseOptionalKeyword(&kw)) {
    if (kw.is(kCatchKeyword)) // handle catch
    else if (kw.is(kFilterKeyword)) // handle filter
    else if (kw.is(kCleanupKeyword)) // handle cleanup
  }
  parseColonType(..)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72006