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

[shraiysh via Phabricator via llvm-commits]

shraiysh added a comment.

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

No, the code is not finished yet. I just wanted reviews on the design. Thanks.

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

It goes into a segmentation fault. I will file a bug.

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

Agreed. Handling the function call is same for both `call` and `invoke` - so, once the function pointers are handled for `call`, it will be exactly the same for `invoke`. I think this syntax is going to work even after the change - it's just that while handling those pointers, the LLVM IR parser in MLIR might have to change a bit - that is what I meant by revision.

> 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. which would correspond to
>  `landingpad catch i8* @_ZTli, filter [0xi8**] undef, catch i8* @_ZTd, filter [1xi8*] @_ZTf `
>  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.

This sounds like a better idea. I will start working on this instead of the current thing.

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

Okay. That makes sense.

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

Sure thing. I will report about those too.

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

Sure, I will do this.

Thanks for the comments @ftynse, @rriddle.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72006