[PATCH] D84602: [MSP430] Expose msp430_builtin calling convention to C code

Mon Aug 10 06:13:43 PDT 2020

aaron.ballman added a reviewer: echristo.
aaron.ballman added a subscriber: echristo.
aaron.ballman added a comment.

Adding @echristo for the debugging info question.

In D84602#2200274 <https://reviews.llvm.org/D84602#2200274>, @atrosinenko wrote:

> 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 <https://reviews.llvm.org/D84602> (this one), D84605 <https://reviews.llvm.org/D84605> and, the most scary of them, D84636 <https://reviews.llvm.org/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.

================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1233
     return llvm::dwarf::DW_CC_LLVM_X86RegCall;
+  // TODO case CC_MSP430Builtin:
   }
----------------
aaron.ballman wrote:
> atrosinenko wrote:
> > aaron.ballman wrote:
> > > 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?
> > > 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`.
> > 
> 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.
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602