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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko updated this revision to Diff 288305.
atrosinenko added a comment.

- 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Specifiers.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/Type.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/MSP430.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/msp430-cc-builtin.c
  clang/test/Sema/attr-msp430.c
  clang/tools/libclang/CXType.cpp
  llvm/lib/Target/MSP430/MSP430ISelLowering.cpp

Index: llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
===
--- llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
+++ llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
@@ -573,6 +573,7 @@
 report_fatal_error("Unsupported calling convention");
   case CallingConv::C:
   case CallingConv::Fast:
+  case CallingConv::MSP430_BUILTIN:
 return LowerCCCArguments(Chain, CallConv, isVarArg, Ins, dl, DAG, InVals);
   case CallingConv::MSP430_INTR:
 if (Ins.empty())
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -666,6 +666,7 @@
   TCALLINGCONV(Swift);
   TCALLINGCONV(PreserveMost);
   TCALLINGCONV(PreserveAll);
+case CC_MSP430Builtin: return CXCallingConv_Unexposed;
 case CC_SpirFunction: return CXCallingConv_Unexposed;
 case CC_OpenCLKernel: return CXCallingConv_Unexposed;
   break;
Index: clang/test/Sema/attr-msp430.c
===
--- clang/test/Sema/attr-msp430.c
+++ clang/test/Sema/attr-msp430.c
@@ -11,3 +11,7 @@
 
 __attribute__((interrupt(0))) void f6(void);
 __attribute__((interrupt(63))) void f7(void);
+
+__attribute__((msp430_builtin)) int t2;   // expected-warning {{'msp430_builtin' only applies to function types; type here is 'int'}}
+__attribute__((msp430_builtin(1))) void f8(long long a, long long b); // expected-error {{'msp430_builtin' attribute takes no arguments}}
+__attribute__((msp430_builtin)) void f9(long long a, long long b);
Index: clang/test/CodeGen/msp430-cc-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/msp430-cc-builtin.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple msp430 -emit-llvm < %s | FileCheck %s
+
+__attribute__((msp430_builtin)) int f(long long x, long long y);
+
+__attribute__((msp430_builtin)) int g(long long x, long long y) {
+  return (int)42;
+}
+// CHECK: define cc94 {{(dso_local )?}}i16 @g(i64 %x, i64 %y)
+
+int caller() {
+  return f(0, 0);
+// CHECK: call cc94 i16 @f
+}
+
+// CHECK: declare cc94 {{(dso_local )?}}i16 @f(i64, i64)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -124,7 +124,8 @@
   case ParsedAttr::AT_Pcs: \
   case ParsedAttr::AT_IntelOclBicc:\
   case ParsedAttr::AT_PreserveMost:\
-  case ParsedAttr::AT_PreserveAll
+  case ParsedAttr::AT_PreserveAll: \
+  case ParsedAttr::AT_MSP430Builtin
 
 // Function type attributes.
 #define FUNCTION_TYPE_ATTRS_CASELIST   \
@@ -7304,6 +7305,8 @@
 return createSimpleAttr(Ctx, Attr);
   case ParsedAttr::AT_PreserveAll:
 return createSimpleAttr(Ctx, Attr);
+  case ParsedAttr::AT_MSP430Builtin:
+return createSimpleAttr(Ctx, Attr);
   }
   llvm_unreachable("unexpected attribute kind!");
 }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4476,6 +4476,9 @@
   case ParsedAttr::AT_PreserveAll:
 D->addAttr(::new (S.Context) PreserveAllAttr(S.Context, AL));
 return;
+  case ParsedAttr::AT_MSP430Builtin:
+D->addAttr(::new (S.Context) MSP430BuiltinAttr(S.Context, AL));
+return;
   default:
 llvm_unreachable("unexpected attribute kind");
   }
@@ -4641,6 +4644,9 @@
   case ParsedAttr::AT_PreserveAll:
 CC = CC_PreserveAll;
 break;
+  case ParsedAttr::AT_MSP430Builtin:
+CC = CC_MSP430Builtin;
+break;
   default: llvm_unreachable("unexpected attribute kind");
   }
 
@@ -7296,6 +7302,7 @@
   case ParsedAttr::AT_PreserveMost:
   case ParsedAttr::AT_PreserveAll:
   case 

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko added a comment.

In D84602#2206512 , @aaron.ballman 
wrote:

> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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 , @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  (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.




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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D84602#2178328 , @atrosinenko wrote:

> In D84602#2176592 , @aaron.ballman 
> wrote:
>
>> 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
>
> - D84602: [MSP430] Expose msp430_builtin calling convention to C code 
> 
> - D84605: [IR][MSP430] Expose the "msp430_builtin" calling convention to .ll 
> 
> - D84636: [RFC] Make the default LibCall implementations from compiler-rt 
> builtins library more customizable 
>
> ... 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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko updated this revision to Diff 281198.
atrosinenko added a comment.

Fix some review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Specifiers.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/Type.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/MSP430.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/msp430-cc-builtin.c
  clang/test/Sema/attr-msp430.c
  clang/tools/libclang/CXType.cpp
  llvm/lib/Target/MSP430/MSP430ISelLowering.cpp

Index: llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
===
--- llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
+++ llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
@@ -573,6 +573,7 @@
 report_fatal_error("Unsupported calling convention");
   case CallingConv::C:
   case CallingConv::Fast:
+  case CallingConv::MSP430_BUILTIN:
 return LowerCCCArguments(Chain, CallConv, isVarArg, Ins, dl, DAG, InVals);
   case CallingConv::MSP430_INTR:
 if (Ins.empty())
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -666,6 +666,7 @@
   TCALLINGCONV(Swift);
   TCALLINGCONV(PreserveMost);
   TCALLINGCONV(PreserveAll);
+case CC_MSP430Builtin: return CXCallingConv_Unexposed;
 case CC_SpirFunction: return CXCallingConv_Unexposed;
 case CC_OpenCLKernel: return CXCallingConv_Unexposed;
   break;
Index: clang/test/Sema/attr-msp430.c
===
--- clang/test/Sema/attr-msp430.c
+++ clang/test/Sema/attr-msp430.c
@@ -11,3 +11,7 @@
 
 __attribute__((interrupt(0))) void f6(void);
 __attribute__((interrupt(63))) void f7(void);
+
+__attribute__((msp430_builtin)) int t2; // expected-warning {{'msp430_builtin' only applies to function types; type here is 'int'}}
+__attribute__((msp430_builtin(1))) void f8(long long a, long long b); // expected-error {{'msp430_builtin' attribute takes no arguments}}
+__attribute__((msp430_builtin)) void f9(long long a, long long b);
Index: clang/test/CodeGen/msp430-cc-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/msp430-cc-builtin.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple msp430 -emit-llvm < %s | FileCheck %s
+
+__attribute__((msp430_builtin)) int f(long long x, long long y);
+
+__attribute__((msp430_builtin)) int g(long long x, long long y) {
+  return (int)42;
+}
+// CHECK: define cc94 {{(dso_local )?}}i16 @g(i64 %x, i64 %y)
+
+int caller()
+{
+  return f(0, 0);
+// CHECK: call cc94 i16 @f
+}
+
+// CHECK: declare cc94 {{(dso_local )?}}i16 @f(i64, i64)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -124,7 +124,8 @@
   case ParsedAttr::AT_Pcs: \
   case ParsedAttr::AT_IntelOclBicc:\
   case ParsedAttr::AT_PreserveMost:\
-  case ParsedAttr::AT_PreserveAll
+  case ParsedAttr::AT_PreserveAll: \
+  case ParsedAttr::AT_MSP430Builtin
 
 // Function type attributes.
 #define FUNCTION_TYPE_ATTRS_CASELIST   \
@@ -7248,6 +7249,8 @@
 return createSimpleAttr(Ctx, Attr);
   case ParsedAttr::AT_PreserveAll:
 return createSimpleAttr(Ctx, Attr);
+  case ParsedAttr::AT_MSP430Builtin:
+return createSimpleAttr(Ctx, Attr);
   }
   llvm_unreachable("unexpected attribute kind!");
 }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4459,6 +4459,9 @@
   case ParsedAttr::AT_PreserveAll:
 D->addAttr(::new (S.Context) PreserveAllAttr(S.Context, AL));
 return;
+  case ParsedAttr::AT_MSP430Builtin:
+D->addAttr(::new (S.Context) MSP430BuiltinAttr(S.Context, AL));
+return;
   default:
 llvm_unreachable("unexpected attribute kind");
   }
@@ -4624,6 +4627,9 @@
   case ParsedAttr::AT_PreserveAll:
 CC = CC_PreserveAll;
 break;
+  case ParsedAttr::AT_MSP430Builtin:
+CC = CC_MSP430Builtin;
+break;
   default: llvm_unreachable("unexpected attribute kind");
   }
 
@@ -7252,6 +7258,7 @@
   case ParsedAttr::AT_PreserveMost:
   case ParsedAttr::AT_PreserveAll:
   case ParsedAttr::AT_AArch64VectorPcs:
+  case ParsedAttr::AT_MSP430Builtin:
 handleCallConvAttr(S, D, AL);
 break;
   case ParsedAttr::AT_Suppress:
Index: 

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko added a comment.

In D84602#2176584 , @rjmccall wrote:

> 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.

In D84602#2176592 , @aaron.ballman 
wrote:

> 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

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

- D84605: [IR][MSP430] Expose the "msp430_builtin" calling convention to .ll 

- D84636: [RFC] Make the default LibCall implementations from compiler-rt 
builtins library more customizable 

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1465
+  let Spellings = [Clang<"msp430_builtin">];
+  let Documentation = [Undocumented];
+}

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.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1233
 return llvm::dwarf::DW_CC_LLVM_X86RegCall;
+  // TODO case CC_MSP430Builtin:
   }

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`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D84602#2176584 , @rjmccall wrote:

> 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. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84602



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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];
+}

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko created this revision.
atrosinenko added reviewers: asl, aaron.ballman, rjmccall.
Herald added subscribers: llvm-commits, hiraditya.
Herald added projects: clang, LLVM.

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).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84602

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Specifiers.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/Type.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/MSP430.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/msp430-cc-builtin.c
  clang/test/Sema/attr-msp430.c
  clang/tools/libclang/CXType.cpp
  llvm/lib/Target/MSP430/MSP430ISelLowering.cpp

Index: llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
===
--- llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
+++ llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
@@ -573,6 +573,7 @@
 report_fatal_error("Unsupported calling convention");
   case CallingConv::C:
   case CallingConv::Fast:
+  case CallingConv::MSP430_BUILTIN:
 return LowerCCCArguments(Chain, CallConv, isVarArg, Ins, dl, DAG, InVals);
   case CallingConv::MSP430_INTR:
 if (Ins.empty())
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -666,6 +666,7 @@
   TCALLINGCONV(Swift);
   TCALLINGCONV(PreserveMost);
   TCALLINGCONV(PreserveAll);
+case CC_MSP430Builtin: return CXCallingConv_Unexposed;
 case CC_SpirFunction: return CXCallingConv_Unexposed;
 case CC_OpenCLKernel: return CXCallingConv_Unexposed;
   break;
Index: clang/test/Sema/attr-msp430.c
===
--- clang/test/Sema/attr-msp430.c
+++ clang/test/Sema/attr-msp430.c
@@ -11,3 +11,6 @@
 
 __attribute__((interrupt(0))) void f6(void);
 __attribute__((interrupt(63))) void f7(void);
+
+__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);
Index: clang/test/CodeGen/msp430-cc-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/msp430-cc-builtin.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple msp430 -emit-llvm < %s | FileCheck %s
+
+__attribute__((msp430_builtin)) int f(long long x, long long y);
+
+__attribute__((msp430_builtin)) int g(long long x, long long y) {
+  return (int)42;
+}
+// CHECK: define cc94 {{(dso_local )?}}i16 @g(i64 %x, i64 %y)
+
+int caller()
+{
+  return f(0, 0);
+// CHECK: call cc94 i16 @f
+}
+
+// CHECK: declare cc94 {{(dso_local )?}}i16 @f(i64, i64)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -124,7 +124,8 @@
   case ParsedAttr::AT_Pcs: \
   case ParsedAttr::AT_IntelOclBicc:\
   case ParsedAttr::AT_PreserveMost:\
-  case ParsedAttr::AT_PreserveAll
+  case ParsedAttr::AT_PreserveAll: \
+  case ParsedAttr::AT_MSP430Builtin
 
 // Function type attributes.
 #define FUNCTION_TYPE_ATTRS_CASELIST   \
@@ -7248,6 +7249,8 @@
 return createSimpleAttr(Ctx, Attr);
   case ParsedAttr::AT_PreserveAll:
 return createSimpleAttr(Ctx, Attr);
+  case ParsedAttr::AT_MSP430Builtin:
+return createSimpleAttr(Ctx, Attr);
   }
   llvm_unreachable("unexpected attribute kind!");
 }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4459,6 +4459,9 @@
   case ParsedAttr::AT_PreserveAll:
 D->addAttr(::new (S.Context) PreserveAllAttr(S.Context, AL));
 return;
+  case ParsedAttr::AT_MSP430Builtin:
+