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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1465
+  let Spellings = [Clang<"msp430_builtin">];
+  let Documentation = [Undocumented];
+}
----------------
atrosinenko wrote:
> aaron.ballman wrote:
> > 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?)
> Initially, it was not exposed and was just emitted according to the [MSP430 EABI document](https://www.ti.com/lit/an/slaa534a/slaa534a.pdf), 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}.
> 
> 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.
> 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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602