Skip to content

Conversation

seven-mile
Copy link
Collaborator

This PR simply adds the calling convention attribute to FuncOp with LLVM Lowering support.

The overall approach follows GlobalLinkageKind: Extend the ODS, parser, printer and lowering pass. When the call conv is C call conv, it's omitted in the output assembly.

@seven-mile
Copy link
Collaborator Author

The CodeGen stuff will come later.

The newly defined CallingConv enumeration resides in the namespace ::mlir::cir, differs from the ::cir::CallingConv from CodeGen skeleton. The latter will also be soon removed, and hopefully all the occurrences of it will be replaced by the one defined in this PR.

Comment on lines 2799 to 2801
def CC_C : I32EnumAttrCase<"C", 1, "cc_c">;
def CC_SpirKernel : I32EnumAttrCase<"SpirKernel", 75, "cc_spir_kernel">;
def CC_SpirFunction : I32EnumAttrCase<"SpirFunction", 76, "cc_spir_function">;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the names in asm. The func op has many keywords right after the mnemonic. We do not have hint to tell "this is a calling convention keyword". Meanwhile it's hard to name the short "c" case. Therefore the prefix cc_ was prepended accordingly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not have hint to tell "this is a calling convention keyword"

Why not add this hint and design the assembly like:

cir.func @foo() cc(c)
cir.func @foo() cc(spir_kernel)
cir.func @foo() cc(spir_function)

Copy link
Collaborator Author

@seven-mile seven-mile Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it out! That's also a very nice option. Other FYI:

  • We already have assembly format like cir.func internal private @_ZL4funcv() extra(#fn_attr). Here internal is linkage and private is symbol visibility. (it's generated by a static function)
  • I'm assuming the postpositional attribute here is in order to align with our existing form extra(#fn_attr). But we may also consider that the conventional position for call conv is always before the name, no matter in the source code, LLVM MLIR dialect or LLVM IR.

Two more schemes here:

cir.func callconv(c) @foo()
cir.func callconv(spir_kernel) @foo()
cir.func callconv(spir_function) @foo()
cir.func callconv_c @foo()
cir.func spir_kernel @foo()
cir.func spir_function @foo()

Edit: callconv_c -> cdecl is also a good name I think!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more schemes here:

I'm down for the first one. We have attributes that specifies what "kind" the function is:

cir.func coroutine @foo()

The spir_kernel and spir_function looks too similar to these kind of attributes and it's better to be explicit about their actual usage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • +1 for having the CC before the symbol name.
  • LLVM uses ccc for the C calling convention. They use the no-separator-'cc'-suffix for others as well, though not for spir_function and spir_kernel.

The spir_kernel and spir_function looks too similar to these kind of attributes and it's better to be explicit about their actual usage.

  • I get where you're coming from, but I'd argue that the assembly format is not user-facing/only consumed by computers and compiler devs, so as long as it can be parsed unambiguously, wouldn't it be fine?
  • I think wrapping the keyword in cc(...), cconv(...), callconv(...), etc. is also fine, but then we probably should wrap linkage and visibility as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more driven towards @Lancern idea:

cir.func @foo() cc(c)
cir.func @foo() cc(spir_kernel)
cir.func @foo() cc(spir_function)

My thoughts:

  • As someone always looking into CIR output, I rather not have to wait more into finding the symbol.
  • visibility(internal) is quite a big name, so I think it justifies being different. Adding a linkage(...) would also be fair, but I feel like linkage information is something you usually also want in the face. I'm also not that worried about being super consistent if there are good reasons to deviate. cc is so tiny that begs to be used hehe.
  • I also think that the most common CC should not be printed (i.e. we should have a default) in order not to look at that when we don't need to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the suggestions. Updated accordingly.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this!

Comment on lines 2799 to 2801
def CC_C : I32EnumAttrCase<"C", 1, "cc_c">;
def CC_SpirKernel : I32EnumAttrCase<"SpirKernel", 75, "cc_spir_kernel">;
def CC_SpirFunction : I32EnumAttrCase<"SpirFunction", 76, "cc_spir_function">;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more driven towards @Lancern idea:

cir.func @foo() cc(c)
cir.func @foo() cc(spir_kernel)
cir.func @foo() cc(spir_function)

My thoughts:

  • As someone always looking into CIR output, I rather not have to wait more into finding the symbol.
  • visibility(internal) is quite a big name, so I think it justifies being different. Adding a linkage(...) would also be fair, but I feel like linkage information is something you usually also want in the face. I'm also not that worried about being super consistent if there are good reasons to deviate. cc is so tiny that begs to be used hehe.
  • I also think that the most common CC should not be printed (i.e. we should have a default) in order not to look at that when we don't need to.

Copy link
Contributor

@jopperm jopperm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!


/// Parse an enum from the keyword, return failure if the keyword is not found.
template <typename EnumTy, typename RetTy = EnumTy>
static ParseResult parseCIRKeyword(AsmParser &parser, RetTy &result) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, probably we can reuse that for other things in this file, can you create an issue so we can track it?

@bcardosolopes bcardosolopes merged commit 53f9d44 into llvm:main Aug 2, 2024
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
…lvm#760)

This PR simply adds the calling convention attribute to FuncOp with LLVM
Lowering support.

The overall approach follows `GlobalLinkageKind`: Extend the ODS,
parser, printer and lowering pass. When the call conv is C call conv,
it's omitted in the output assembly.

---------

Co-authored-by: Bruno Cardoso Lopes <[email protected]>
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
…lvm#760)

This PR simply adds the calling convention attribute to FuncOp with LLVM
Lowering support.

The overall approach follows `GlobalLinkageKind`: Extend the ODS,
parser, printer and lowering pass. When the call conv is C call conv,
it's omitted in the output assembly.

---------

Co-authored-by: Bruno Cardoso Lopes <[email protected]>
smeenai pushed a commit to smeenai/clangir that referenced this pull request Oct 9, 2024
…lvm#760)

This PR simply adds the calling convention attribute to FuncOp with LLVM
Lowering support.

The overall approach follows `GlobalLinkageKind`: Extend the ODS,
parser, printer and lowering pass. When the call conv is C call conv,
it's omitted in the output assembly.

---------

Co-authored-by: Bruno Cardoso Lopes <[email protected]>
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
…lvm#760)

This PR simply adds the calling convention attribute to FuncOp with LLVM
Lowering support.

The overall approach follows `GlobalLinkageKind`: Extend the ODS,
parser, printer and lowering pass. When the call conv is C call conv,
it's omitted in the output assembly.

---------

Co-authored-by: Bruno Cardoso Lopes <[email protected]>
lanza pushed a commit that referenced this pull request Nov 5, 2024
…760)

This PR simply adds the calling convention attribute to FuncOp with LLVM
Lowering support.

The overall approach follows `GlobalLinkageKind`: Extend the ODS,
parser, printer and lowering pass. When the call conv is C call conv,
it's omitted in the output assembly.

---------

Co-authored-by: Bruno Cardoso Lopes <[email protected]>
lanza pushed a commit that referenced this pull request Mar 18, 2025
…760)

This PR simply adds the calling convention attribute to FuncOp with LLVM
Lowering support.

The overall approach follows `GlobalLinkageKind`: Extend the ODS,
parser, printer and lowering pass. When the call conv is C call conv,
it's omitted in the output assembly.

---------

Co-authored-by: Bruno Cardoso Lopes <[email protected]>
terapines-osc-cir pushed a commit to Terapines/clangir that referenced this pull request Sep 2, 2025
…lvm#760)

This PR simply adds the calling convention attribute to FuncOp with LLVM
Lowering support.

The overall approach follows `GlobalLinkageKind`: Extend the ODS,
parser, printer and lowering pass. When the call conv is C call conv,
it's omitted in the output assembly.

---------

Co-authored-by: Bruno Cardoso Lopes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants