This is an archive of the discontinued LLVM Phabricator instance.

[MSP430] Expose msp430_builtin calling convention to C code
Needs ReviewPublic

Authored by atrosinenko on Jul 26 2020, 11:43 AM.

Details

Summary

According to MSP430 EABI document, Section 6.3, some of compiler helper functions require a special calling convention that is used for some of LibCalls accepting two 64-bit arguments.

As part of porting the compiler-rt builtins library to MSP430 I need to not only internally emit calls to these LibCalls but also to explicitly declare such functions in C code (for explicilty calling them from the unit test code) and to implement them in C (when I do not want to replace the generic C implementation with custom assembler code).

References: How to add an attribute chapter covers basics of adding a new attribute (not specifically calling convention-related).

Diff Detail

Unit TestsFailed

Event Timeline

atrosinenko created this revision.Jul 26 2020, 11:43 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 26 2020, 11:43 AM
aaron.ballman added inline comments.Jul 27 2020, 11:15 AM
clang/include/clang/Basic/Attr.td
1479

No new, undocumented attributes, please. Or is this attribute not expected to be used by users? (If it's not to be used by end users, is there a way we can skip exposing the attribute in the frontend and automatically generate the LLVM calling convention when lowering the builtin?)

clang/lib/Basic/Targets/MSP430.h
100

I think the lint warning about the formatting is actually correct in this case (but not in the other cases, unfortunately).

clang/lib/CodeGen/CGDebugInfo.cpp
1250

This is likely to cause -Werror failures because the switch won't be fully covered. Are you planning to do this TODO as part of this patch, or in a follow up?

clang/test/Sema/attr-msp430.c
15

Can you also add a sema test that the attribute accepts no arguments?

Is there only one special calling convention, or is there any chance that different builtin functions would use different conventions?

I don't have a problem with introducing a new convention even if it's only used for "builtin" functions.

I don't have a problem with introducing a new convention even if it's only used for "builtin" functions.

To be clear, I also don't have a problem with it, but if users aren't supposed to be writing this attribute themselves and if we can apply the calling convention from markings in a .def file instead, I think it could be a cleaner approach to go that route instead. There's a lot of "ifs" in that sentence though. :-)

atrosinenko added inline comments.Jul 28 2020, 4:12 AM
clang/include/clang/Basic/Attr.td
1479

Initially, it was not exposed and was just emitted according to the MSP430 EABI document, Section 6.3, as calls to libgcc. Now, when porting the builtins library of compiler-rt, I had to be able to

  • define some of functions as a library function with a special calling convention
  • declare external functions to be explicitly called from corresponding unit tests

So, it is not expected to mimic some existing attribute or be used by end users. Frankly speaking, it is possible it can change the meaning when finally wiring together all parts of MSP430 compiler-rt port. Ultimately, this is expected to be wired into D84636: [RFC] Make the default LibCall implementations from compiler-rt builtins library more customizable.

At the first glance, msp430-gcc just knows these functions by names and sets CC accordingly. This could technically solve my problem as well but looks quite hackish to me.

clang/lib/CodeGen/CGDebugInfo.cpp
1250

Are you planning to do this TODO as part of this patch, or in a follow up?

It depends on how large that change would be.

I first need to find out how such calling convention identifiers are issued (or maybe there already exist one from GCC). I see they are declared inside the llvm/include/llvm/BinaryFormat/Dwarf.def.

aaron.ballman added inline comments.Jul 28 2020, 4:28 AM
clang/include/clang/Basic/Attr.td
1479

At the first glance, msp430-gcc just knows these functions by names and sets CC accordingly. This could technically solve my problem as well but looks quite hackish to me.

I was thinking that MSP430 would have a BuiltinsMSP430.def file in include/clang/Basic, and you could specify the attribute on just the builtins that matter from within that file. But I notice there is no such file, so perhaps that idea won't work as well for you. If you add such a file, I think you'd attach your calling convention attributes somewhere near Sema::AddKnownFunctionAttributes() but you may have to do some work for the function type itself.

If we keep the explicit attribute, I'm fine with it being undocumented if users aren't supposed to write it themselves, but would appreciate some comments on the attribute explaining that.

clang/lib/CodeGen/CGDebugInfo.cpp
1250

I'm not certain how large the change would be (and I definitely don't insist on a full implementation), but you should ensure the switch is fully covered so that you don't break bots when the patch lands.

Is there only one special calling convention, or is there any chance that different builtin functions would use different conventions?

It depends on how to define "builtin calling convention". Now it is in fact a "CC for builtin functions with two 64-bit arguments listed in Section 6.3 of EABI document". MSP430 EABI defines one such CC applied to a small subset of compiler helper functions, while others use the traditional CC like all other functions.

To be clear, I also don't have a problem with it, but if users aren't supposed to be writing this attribute themselves and if we can apply the calling convention from markings in a .def file instead, I think it could be a cleaner approach to go that route instead. There's a lot of "ifs" in that sentence though. :-)

Do you mean detecting these functions by their names, like GCC does? If so, that trick would replace all the

... provided this is considered as a generally suggested solution across the codebase, not a hack :)

To be clear, I also don't have a problem with it, but if users aren't supposed to be writing this attribute themselves and if we can apply the calling convention from markings in a .def file instead, I think it could be a cleaner approach to go that route instead. There's a lot of "ifs" in that sentence though. :-)

Do you mean detecting these functions by their names, like GCC does? If so, that trick would replace all the

... provided this is considered as a generally suggested solution across the codebase, not a hack :)

I was envisioning specifying the builtins in a .def file like we do with other builtins, but allowing that .def file to specify optional calling convention information on a per-function basis (like we already support other attributes). So there would still be an LLVM attribute to be used, but there would not be a user-facing Clang attribute that users could misapply to their own code. So the attribute definition here would continue to exist, but the attribute would have no spelling associated with it. We do this with some other attributes, like AlignMac68kAttr or MaxFieldAlignmentAttr.

Fix some review comments.

I suspect there might be some terminology clash, so clarifying a bit just in case.

It was probably better to refer to these functions as LibCalls - functions from the compiler support library (such as libgcc) such as __adddf3. While there are __popcount[sdt]i2 among them, most of the times they are implicitly called when the high-level code performs division on __uint128, for example. Unfortunately, they reside under compiler-rt/lib/builtins - so that name was used... :) So, if I get it right, you have proposed something like clang/include/clang/Basic/BuiltinsMips.def and those are another kind of builtins.

The CallingConv::MSP430_BUILTIN was the name of a calling convention that the MSP430 codegen of LLVM had to use for a small subset of those support functions, as defined by MSP430 EABI. While it can be useful to add some other CC for those functions for internal use by compiler-rt someday, now there are only two CCs defined for MSP430 LibCalls: that "special convention" for 13 functions only with two 64-bit arguments (including 64-bit integer shifts) and the CallingConv::C for everything else both from the support library and regular object files.

Technically, these functions could probably be listed by names somewhere in the backend code, completely detangling the upstreaming of MSP430 compiler-rt port from D84602 (this one), D84605 and, the most scary of them, D84636. That may probably look a bit hackish, though.

aaron.ballman added a subscriber: echristo.

Adding @echristo for the debugging info question.

I suspect there might be some terminology clash, so clarifying a bit just in case.

Thank you for the explanations, they help!

It was probably better to refer to these functions as LibCalls - functions from the compiler support library (such as libgcc) such as __adddf3. While there are __popcount[sdt]i2 among them, most of the times they are implicitly called when the high-level code performs division on __uint128, for example. Unfortunately, they reside under compiler-rt/lib/builtins - so that name was used... :) So, if I get it right, you have proposed something like clang/include/clang/Basic/BuiltinsMips.def and those are another kind of builtins.

Ah! That explains my confusion -- I was indeed thinking of something like BuiltinMips.def and hadn't realized this was a different kind of builtin.

The CallingConv::MSP430_BUILTIN was the name of a calling convention that the MSP430 codegen of LLVM had to use for a small subset of those support functions, as defined by MSP430 EABI. While it can be useful to add some other CC for those functions for internal use by compiler-rt someday, now there are only two CCs defined for MSP430 LibCalls: that "special convention" for 13 functions only with two 64-bit arguments (including 64-bit integer shifts) and the CallingConv::C for everything else both from the support library and regular object files.

Technically, these functions could probably be listed by names somewhere in the backend code, completely detangling the upstreaming of MSP430 compiler-rt port from D84602 (this one), D84605 and, the most scary of them, D84636. That may probably look a bit hackish, though.

Given that these builtins are defined in source files rather than in a .td file, I agree that would be rather hackish and using syntax would be more appropriate.

clang/lib/CodeGen/CGDebugInfo.cpp
1250

I still think this case needs some work to keep the bots happy. I would probably go with:

case CC_MSP430Builtin:
  // FIXME: Currently unsupported
  break;

But perhaps @echristo has opinions since this involves debugging information.

I still think this case needs some work to keep the bots happy. I would probably go with:

case CC_MSP430Builtin:

// FIXME: Currently unsupported
break;

But perhaps @echristo has opinions since this involves debugging information.

Agree, unless there exist some straightforward way to add a new vendor-specific extension to include/llvm/BinaryFormat/Dwarf.def. But I suspect there would be some standardization stuff and not sure it will actually be that useful.

  • Rebase onto current master branch
  • Add a dummy switch case with FIXME, as suggested by @aaron.ballman
  • Applied a couple of style fixes proposed by linter

Pinging @echristo in case there is some trivial way to allocate a calling convention ID (but probably there is not, unless GCC allocated one already). As I understand, without this ID the debugger may show some misleading output for a limited number of LibCalls. On the other hand, it will not help either, unless the debugger knows this ID.