Page MenuHomePhabricator

[mlir][LLVM] Add support for Calling Convention in LLVMFuncOp
ClosedPublic

Authored by alexbatashev on May 22 2022, 2:46 AM.

Details

Summary

This patch adds support for Calling Convention attribute in LLVM
dialect, including enums, custom syntax and import from LLVM IR.
Additionally fix import of dso_local attribute.

Diff Detail

Event Timeline

alexbatashev created this revision.May 22 2022, 2:46 AM
alexbatashev requested review of this revision.May 22 2022, 2:46 AM
ftynse added inline comments.May 25 2022, 6:10 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td
237–242

How is this different from LLVM_EnumAttr and why it is necessary?

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
190

Nit: the case difference in naming (getCconv rather than the supposed getCConv) feels annoying here. I understand it is likely generated, do you have an idea where so we can fix it?

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

Nit: do we want to print something like INVALID_cc_ instead so it is clear something went wrong?

Thanks, @ftynse

mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td
237–242

Unlike many other enums in LLVM, this one is anonymous and it resides in CallingConv namespace (see https://llvm.org/doxygen/CallingConv_8h_source.html#l00024). Since LLVM_EnumAttr requires a class name, and here there's none, I had to come up with something, that would be compatible with C-style enums. I'd be happy if someone can suggest anything to make it work without unnecessary complexity.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
190

Yes, this is very autogenerated, and I have no idea how to fix that. I'd be happy to do so if someone has any suggestions.

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

Technically speaking, that's not invalid, and LLVM allows use of arbitrary calling conventions. Not sure it makes much sense to preserve this capability in MLIR. Printing INVALID_ prefix makes sense to me. Let me know if you still want me to apply this suggestion.

jpienaar added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
190

It is changed from what would have been cconv() here to getCconv(). If you want getCConv, you'd need to either start with CConv or c_conv (well you could do cConv too but that feels weirder) for the generator.

Apply some code review comments

alexbatashev added inline comments.May 25 2022, 6:28 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
190

Thanks, this helps, indeed.

What is your strategy against divergence? Could you put a link into the td file?

ftynse accepted this revision.May 25 2022, 6:39 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td
237–242

Let's just add a comment explaining that this is to be used for non-class enums, but is functionally identical to LLVM_EnumAttr otherwise.

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

I suppose this needs to be p << getCConvAttr() to avoid asserting in stringify for unsupported enum values.

2963

Ultimately, we want to support everything LLVM does, but I'm totally fine waiting until we hit the actual use case for this. In the meantime, consistency is important, and since the parser would reject unknown values, we should treat them as invalid.

2973

Please add a test for the user-visible error message.

Also nit: most error messages start with the lower case.

This revision is now accepted and ready to land.May 25 2022, 6:39 AM

Apply review comments and fix a few bugs in the meantime

alexbatashev marked 3 inline comments as done.May 25 2022, 7:15 AM

Thanks everyone for comments. I've added link to original Calling Convention enum and tried to incorporate review feedback. I do not expect keeping this in sync with vanilla LLVM to be a problem: no one should add new calling conventions to MLIR dialect only, and if there's a new calling convention in LLVM, someone would face the discrepancy sooner or later.

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

Since this is an existing attribute, constructed from an enum class, it can't be illegal. So, stringify should never assert.