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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1465
+  let Spellings = [Clang<"msp430_builtin">];
+  let Documentation = [Undocumented];
+}
----------------
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?)


================
Comment at: clang/lib/Basic/Targets/MSP430.h:100
+
+  CallingConvCheckResult checkCallingConvention(CallingConv CC) const override {
+    switch (CC) {
----------------
I think the lint warning about the formatting is actually correct in this case (but not in the other cases, unfortunately).


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


================
Comment at: clang/test/Sema/attr-msp430.c:15
+
+__attribute__((msp430_builtin)) int t2; // expected-warning {{'msp430_builtin' only applies to function types; type here is 'int'}}
+__attribute__((msp430_builtin)) void f8(long long a, long long b);
----------------
Can you also add a sema test that the attribute accepts no arguments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602