[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGec809e4cfe0b: PR47372: Fix Lambda invoker calling 
conventions (authored by erichkeane).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89559

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/test/CodeGenCXX/lambda-conversion-op-cc.cpp
  clang/test/SemaCXX/lambda-conversion-op-cc.cpp

Index: clang/test/SemaCXX/lambda-conversion-op-cc.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-conversion-op-cc.cpp
@@ -0,0 +1,190 @@
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc %s -verify -DBAD_CONVERSION
+// RUN: %clang_cc1 -fsyntax-only -triple i386-windows-pc %s -verify -DBAD_CONVERSION -DWIN32
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc %s -ast-dump | FileCheck %s --check-prefixes=CHECK,LIN64,NODEF
+// RUN: %clang_cc1 -fsyntax-only -triple i386-windows-pc %s -ast-dump -DWIN32 | FileCheck %s --check-prefixes=CHECK,WIN32,NODEF
+
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc -fdefault-calling-conv=vectorcall %s -verify -DBAD_VEC_CONVERS
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc -fdefault-calling-conv=vectorcall %s -ast-dump | FileCheck %s --check-prefixes=CHECK,VECTDEF
+
+void useage() {
+  auto normal = [](int, float, double) {};// #1
+  auto vectorcall = [](int, float, double) __attribute__((vectorcall)){}; // #2
+#ifdef WIN32
+  auto thiscall = [](int, float, double) __attribute__((thiscall)){}; // #3
+#endif// WIN32
+  auto cdecl = [](int, float, double) __attribute__((cdecl)){};
+
+  auto genericlambda = [](auto a) {};  // #4
+  auto genericvectorcalllambda = [](auto a) __attribute__((vectorcall)){}; // #5
+
+  // None of these should be ambiguous.
+  (void)+normal;
+  (void)+vectorcall;
+#ifdef WIN32
+  (void)+thiscall;
+#endif // WIN32
+  (void)+cdecl;
+
+#ifdef BAD_CONVERSION
+  // expected-error-re@+1 {{invalid argument type {{.*}} to unary expression}}
+  (void)+genericlambda;
+  // expected-error-re@+1 {{invalid argument type {{.*}} to unary expression}}
+  (void)+genericvectorcalllambda;
+#endif // BAD_CONVERSION
+
+  // CHECK: VarDecl {{.*}} normal '
+  // CHECK: LambdaExpr
+  // WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+  // LIN64: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+  // VECTDEF: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+  // NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+  // NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+  // VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+  // VECTDEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+  // CHECK: VarDecl {{.*}} vectorcall '
+  // CHECK: LambdaExpr
+  // CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((vectorcall)) const'
+  // CHECK: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+  // CHECK: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+  // WIN32: VarDecl {{.*}} thiscall '
+  // WIN32: LambdaExpr
+  // WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+  // WIN32: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+  // WIN32: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+
+  // CHECK: VarDecl {{.*}} cdecl '
+  // CHECK: LambdaExpr
+  // CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+  // NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+  // NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+  // VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+  // VECTDEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+  // CHECK: VarDecl {{.*}} genericlambda '
+  // CHECK: LambdaExpr
+  //
+  // CHECK: FunctionTemplateDecl {{.*}} operator()
+  // LIN64: CXXMethodDecl {{.*}} operator() 'auto (auto) const' inline
+  // LIN64: CXXMethodDecl {{.*}} operator() 'void (char) const' inline
+  // LIN64: CXXMethodDecl {{.*}} operator() 'void (int) const' inline
+  // WIN32: 

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Alright, LGTM.




Comment at: clang/include/clang/AST/DeclCXX.h:1013
   CXXMethodDecl *getLambdaStaticInvoker() const;
+  CXXMethodDecl *getLambdaStaticInvoker(CallingConv CC) const;
+

erichkeane wrote:
> erichkeane wrote:
> > rjmccall wrote:
> > > Probably worth clarifying in the comment which invoker is returned by the 
> > > no-arguments variant.
> > I just looked and the first is actually only used 1x!  I think I can just 
> > replace that usage and remove the former overload if that is acceptable to 
> > you.
> On second thought... I might leave this alone.  Otherwise the ItaniumMangler 
> basically needs to reproduce the functionality.  I think the improved comment 
> is an appropriate change here.
Fine by me.



Comment at: clang/lib/AST/DeclCXX.cpp:1550
+ return FTy->getCallConv() == CC;
+   });
+

erichkeane wrote:
> rjmccall wrote:
> > This seems both rather more complicated and rather more expensive than a 
> > simple loop which assigns to a local pointer.
> Do you mean the filtering copy-if?  The idea was to make it so that the 
> assert below still applies.  If that isn't necessary, this DOES simplify to a 
> std::find and a return.
I mean that if you really want to assert that, you could do so by checking 
whether the existing function is the same as what you've already found; you 
don't need a vector.  But just not checking that seems fine to me.


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 301746.
erichkeane marked an inline comment as done.
erichkeane added a comment.

@rjmccall : Think I got everything, is this what you mean?


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

https://reviews.llvm.org/D89559

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/test/CodeGenCXX/lambda-conversion-op-cc.cpp
  clang/test/SemaCXX/lambda-conversion-op-cc.cpp

Index: clang/test/SemaCXX/lambda-conversion-op-cc.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-conversion-op-cc.cpp
@@ -0,0 +1,190 @@
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc %s -verify -DBAD_CONVERSION
+// RUN: %clang_cc1 -fsyntax-only -triple i386-windows-pc %s -verify -DBAD_CONVERSION -DWIN32
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc %s -ast-dump | FileCheck %s --check-prefixes=CHECK,LIN64,NODEF
+// RUN: %clang_cc1 -fsyntax-only -triple i386-windows-pc %s -ast-dump -DWIN32 | FileCheck %s --check-prefixes=CHECK,WIN32,NODEF
+
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc -fdefault-calling-conv=vectorcall %s -verify -DBAD_VEC_CONVERS
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc -fdefault-calling-conv=vectorcall %s -ast-dump | FileCheck %s --check-prefixes=CHECK,VECTDEF
+
+void useage() {
+  auto normal = [](int, float, double) {};// #1
+  auto vectorcall = [](int, float, double) __attribute__((vectorcall)){}; // #2
+#ifdef WIN32
+  auto thiscall = [](int, float, double) __attribute__((thiscall)){}; // #3
+#endif// WIN32
+  auto cdecl = [](int, float, double) __attribute__((cdecl)){};
+
+  auto genericlambda = [](auto a) {};  // #4
+  auto genericvectorcalllambda = [](auto a) __attribute__((vectorcall)){}; // #5
+
+  // None of these should be ambiguous.
+  (void)+normal;
+  (void)+vectorcall;
+#ifdef WIN32
+  (void)+thiscall;
+#endif // WIN32
+  (void)+cdecl;
+
+#ifdef BAD_CONVERSION
+  // expected-error-re@+1 {{invalid argument type {{.*}} to unary expression}}
+  (void)+genericlambda;
+  // expected-error-re@+1 {{invalid argument type {{.*}} to unary expression}}
+  (void)+genericvectorcalllambda;
+#endif // BAD_CONVERSION
+
+  // CHECK: VarDecl {{.*}} normal '
+  // CHECK: LambdaExpr
+  // WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+  // LIN64: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+  // VECTDEF: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+  // NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+  // NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+  // VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+  // VECTDEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+  // CHECK: VarDecl {{.*}} vectorcall '
+  // CHECK: LambdaExpr
+  // CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((vectorcall)) const'
+  // CHECK: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+  // CHECK: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+  // WIN32: VarDecl {{.*}} thiscall '
+  // WIN32: LambdaExpr
+  // WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+  // WIN32: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+  // WIN32: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+
+  // CHECK: VarDecl {{.*}} cdecl '
+  // CHECK: LambdaExpr
+  // CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+  // NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+  // NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+  // VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+  // VECTDEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+  // CHECK: VarDecl {{.*}} genericlambda '
+  // CHECK: LambdaExpr
+  //
+  // CHECK: FunctionTemplateDecl {{.*}} operator()
+  // LIN64: CXXMethodDecl {{.*}} operator() 'auto (auto) const' inline
+  // LIN64: CXXMethodDecl {{.*}} operator() 'void (char) const' inline
+  // LIN64: CXXMethodDecl {{.*}} operator() 'void (int) const' inline
+  // WIN32: CXXMethodDecl {{.*}} operator() 'auto (auto) 

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:1013
   CXXMethodDecl *getLambdaStaticInvoker() const;
+  CXXMethodDecl *getLambdaStaticInvoker(CallingConv CC) const;
+

erichkeane wrote:
> rjmccall wrote:
> > Probably worth clarifying in the comment which invoker is returned by the 
> > no-arguments variant.
> I just looked and the first is actually only used 1x!  I think I can just 
> replace that usage and remove the former overload if that is acceptable to 
> you.
On second thought... I might leave this alone.  Otherwise the ItaniumMangler 
basically needs to reproduce the functionality.  I think the improved comment 
is an appropriate change here.


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:1013
   CXXMethodDecl *getLambdaStaticInvoker() const;
+  CXXMethodDecl *getLambdaStaticInvoker(CallingConv CC) const;
+

rjmccall wrote:
> Probably worth clarifying in the comment which invoker is returned by the 
> no-arguments variant.
I just looked and the first is actually only used 1x!  I think I can just 
replace that usage and remove the former overload if that is acceptable to you.



Comment at: clang/lib/AST/DeclCXX.cpp:1550
+ return FTy->getCallConv() == CC;
+   });
+

rjmccall wrote:
> This seems both rather more complicated and rather more expensive than a 
> simple loop which assigns to a local pointer.
Do you mean the filtering copy-if?  The idea was to make it so that the assert 
below still applies.  If that isn't necessary, this DOES simplify to a 
std::find and a return.



Comment at: clang/lib/AST/MicrosoftMangle.cpp:2268
+} else if (const auto *AT = dyn_cast_or_null(
+   ResultType->getContainedAutoType())) {
   Out << '?';

rjmccall wrote:
> So, this is changing the mangling of lambda conversion operators.  Is this 
> what MSVC does?
Yes, this mangles the 'return type' of the conversion operator, which MSVC does 
as well.  We have an incompatible mangling (already) that skipped this because 
the type has some sort of deduction in it (to protect against a type that is 
defined in terms of the same type).





Comment at: clang/lib/Sema/SemaLambda.cpp:1467
+  llvm::SmallVector Conventions =
+  getLambdaConversionFunctionCallConv(S, CallOpProto);
+

rjmccall wrote:
> Can we make this call a callback or something rather than returning an array 
> that almost always has one element?
Sure!  Can do.



Comment at: clang/lib/Sema/SemaLambda.cpp:1278
+  if (CallOpCC == DefaultMember)
+return DefaultFree;
+  return CallOpCC;

rjmccall wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > rsmith wrote:
> > > > erichkeane wrote:
> > > > > rjmccall wrote:
> > > > > > ...I made this comment in my first review, but Phabricator threw it 
> > > > > > away.
> > > > > > 
> > > > > > The attributes let you explicitly request the default method CC, 
> > > > > > right?  I think you need to check for an explicit attribute rather 
> > > > > > than just checking CC identity.  There should be an AttributedType 
> > > > > > in the sugar.
> > > > > They do, but I can't seem to find a way to find them.  The calling 
> > > > > convention is already merged into the functiontype by the time we get 
> > > > > here, the AttributedType isn't accessible.
> > > > > 
> > > > > So it seems we don't distinguish between "modified by attribute", 
> > > > > "modified-default by command line", and "modified-default by 
> > > > > TargetInfo."
> > > > > 
> > > > > That said, I somewhat think this is the right thing to do anyway.  If 
> > > > > you're on a platform where the default call convention is different 
> > > > > between a free-function and member-function, I'd think that this is 
> > > > > what you MEAN...
> > > > The `AttributedType` should be present in the type on the 
> > > > `TypeSourceInfo` for the call operator. It might not be present on the 
> > > > type retrieved by `getType()`, though.
> > > > 
> > > > Concretely, what targets have different calling conventions for member 
> > > > versus non-member functions, and what do those calling conventions do 
> > > > differently? (For example, if the default member calling convention 
> > > > treats `this` differently, it doesn't seem reasonable to apply that to 
> > > > the static invoker...)
> > > Answering my own question: the only case where the default member calling 
> > > convention is different from the default non-member calling convention is 
> > > for MinGW on 32-bit x86, where members use `thiscall` by default (which 
> > > passes the first parameter in `%ecx`).
> > > 
> > > Is it reasonable for `[] [[clang::thiscall]] {}` to result in a static 
> > > invoker using the `thiscall` calling convention? Intuitively I'd say no, 
> > > but there's not really anything specific to member functions in 
> > > `thiscall` -- it just means that we pass the first argument in a 
> > > register. (However, the first argument of the lambda and the first 
> > > argument of its static invoker are different, so it's still somewhat 
> > > unclear whether inheriting `thiscall` is appropriate. But usually for any 
> > > given lambda only one of the two is actually used.)
> > > 
> > > But there's another quirk here: the default non-member calling convention 
> > > can be set on the command line with `-fdefault-calling-conv=` (and a few 
> > > other flags such as `-mrtd`). This doesn't affect member functions by 
> > > default. So what should happen if this is compiled with 
> > > 

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/AST/DeclCXX.h:1013
   CXXMethodDecl *getLambdaStaticInvoker() const;
+  CXXMethodDecl *getLambdaStaticInvoker(CallingConv CC) const;
+

Probably worth clarifying in the comment which invoker is returned by the 
no-arguments variant.



Comment at: clang/lib/AST/DeclCXX.cpp:1550
+ return FTy->getCallConv() == CC;
+   });
+

This seems both rather more complicated and rather more expensive than a simple 
loop which assigns to a local pointer.



Comment at: clang/lib/AST/DeclCXX.cpp:1568
+return llvm::find_if(Invokers, MDEqual) != Invokers.end();
+  }
+

Can you clarify how this method is semantically different from:

```
  assert(MD->getParent() == this);
  return isLambda() && MD->getName() == getLambdaStaticInvokerName();
```

other than probably inadvertently working with a method from a completely 
different class?



Comment at: clang/lib/AST/MicrosoftMangle.cpp:2268
+} else if (const auto *AT = dyn_cast_or_null(
+   ResultType->getContainedAutoType())) {
   Out << '?';

So, this is changing the mangling of lambda conversion operators.  Is this what 
MSVC does?



Comment at: clang/lib/Sema/SemaLambda.cpp:1467
+  llvm::SmallVector Conventions =
+  getLambdaConversionFunctionCallConv(S, CallOpProto);
+

Can we make this call a callback or something rather than returning an array 
that almost always has one element?



Comment at: clang/lib/Sema/SemaLambda.cpp:1278
+  if (CallOpCC == DefaultMember)
+return DefaultFree;
+  return CallOpCC;

erichkeane wrote:
> rsmith wrote:
> > rsmith wrote:
> > > erichkeane wrote:
> > > > rjmccall wrote:
> > > > > ...I made this comment in my first review, but Phabricator threw it 
> > > > > away.
> > > > > 
> > > > > The attributes let you explicitly request the default method CC, 
> > > > > right?  I think you need to check for an explicit attribute rather 
> > > > > than just checking CC identity.  There should be an AttributedType in 
> > > > > the sugar.
> > > > They do, but I can't seem to find a way to find them.  The calling 
> > > > convention is already merged into the functiontype by the time we get 
> > > > here, the AttributedType isn't accessible.
> > > > 
> > > > So it seems we don't distinguish between "modified by attribute", 
> > > > "modified-default by command line", and "modified-default by 
> > > > TargetInfo."
> > > > 
> > > > That said, I somewhat think this is the right thing to do anyway.  If 
> > > > you're on a platform where the default call convention is different 
> > > > between a free-function and member-function, I'd think that this is 
> > > > what you MEAN...
> > > The `AttributedType` should be present in the type on the 
> > > `TypeSourceInfo` for the call operator. It might not be present on the 
> > > type retrieved by `getType()`, though.
> > > 
> > > Concretely, what targets have different calling conventions for member 
> > > versus non-member functions, and what do those calling conventions do 
> > > differently? (For example, if the default member calling convention 
> > > treats `this` differently, it doesn't seem reasonable to apply that to 
> > > the static invoker...)
> > Answering my own question: the only case where the default member calling 
> > convention is different from the default non-member calling convention is 
> > for MinGW on 32-bit x86, where members use `thiscall` by default (which 
> > passes the first parameter in `%ecx`).
> > 
> > Is it reasonable for `[] [[clang::thiscall]] {}` to result in a static 
> > invoker using the `thiscall` calling convention? Intuitively I'd say no, 
> > but there's not really anything specific to member functions in `thiscall` 
> > -- it just means that we pass the first argument in a register. (However, 
> > the first argument of the lambda and the first argument of its static 
> > invoker are different, so it's still somewhat unclear whether inheriting 
> > `thiscall` is appropriate. But usually for any given lambda only one of the 
> > two is actually used.)
> > 
> > But there's another quirk here: the default non-member calling convention 
> > can be set on the command line with `-fdefault-calling-conv=` (and a few 
> > other flags such as `-mrtd`). This doesn't affect member functions by 
> > default. So what should happen if this is compiled with 
> > `-fdefault-calling-conv=stdcall` (assuming the default calling convention 
> > is otherwise `cdecl`):
> > 
> > ```
> > auto *p0 = [] [[clang::stdcal]] {};
> > auto *p1 = [] {};
> > auto *p2 = [] [[clang::cdecl]] {};
> > ```
> > 
> > `p0` seems easy: the default non-member calling convention and the explicit 
> > calling convention are the same. The invoker should be `stdcall`.
> > 
> > For `p1`, the 

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Ping!


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 300396.
erichkeane added a comment.

Alright, I have the multi-emit dealt with, and the problems that come with that 
solved.  This doesn't do the MSVC-emit-for-all-CCs thing, but I intend to do 
that in a separate patch with the same infrastructure.


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

https://reviews.llvm.org/D89559

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/test/CodeGenCXX/lambda-conversion-op-cc.cpp
  clang/test/SemaCXX/lambda-conversion-op-cc.cpp

Index: clang/test/SemaCXX/lambda-conversion-op-cc.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-conversion-op-cc.cpp
@@ -0,0 +1,190 @@
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc %s -verify -DBAD_CONVERSION
+// RUN: %clang_cc1 -fsyntax-only -triple i386-windows-pc %s -verify -DBAD_CONVERSION -DWIN32
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc %s -ast-dump | FileCheck %s --check-prefixes=CHECK,LIN64,NODEF
+// RUN: %clang_cc1 -fsyntax-only -triple i386-windows-pc %s -ast-dump -DWIN32 | FileCheck %s --check-prefixes=CHECK,WIN32,NODEF
+
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc -fdefault-calling-conv=vectorcall %s -verify -DBAD_VEC_CONVERS
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc -fdefault-calling-conv=vectorcall %s -ast-dump | FileCheck %s --check-prefixes=CHECK,VECTDEF
+
+void useage() {
+  auto normal = [](int, float, double) {};// #1
+  auto vectorcall = [](int, float, double) __attribute__((vectorcall)){}; // #2
+#ifdef WIN32
+  auto thiscall = [](int, float, double) __attribute__((thiscall)){}; // #3
+#endif// WIN32
+  auto cdecl = [](int, float, double) __attribute__((cdecl)){};
+
+  auto genericlambda = [](auto a) {};  // #4
+  auto genericvectorcalllambda = [](auto a) __attribute__((vectorcall)){}; // #5
+
+  // None of these should be ambiguous.
+  (void)+normal;
+  (void)+vectorcall;
+#ifdef WIN32
+  (void)+thiscall;
+#endif // WIN32
+  (void)+cdecl;
+
+#ifdef BAD_CONVERSION
+  // expected-error-re@+1 {{invalid argument type {{.*}} to unary expression}}
+  (void)+genericlambda;
+  // expected-error-re@+1 {{invalid argument type {{.*}} to unary expression}}
+  (void)+genericvectorcalllambda;
+#endif // BAD_CONVERSION
+
+  // CHECK: VarDecl {{.*}} normal '
+  // CHECK: LambdaExpr
+  // WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+  // LIN64: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+  // VECTDEF: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+  // NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+  // NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+  // VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+  // VECTDEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+  // CHECK: VarDecl {{.*}} vectorcall '
+  // CHECK: LambdaExpr
+  // CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((vectorcall)) const'
+  // CHECK: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+  // CHECK: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+  // WIN32: VarDecl {{.*}} thiscall '
+  // WIN32: LambdaExpr
+  // WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+  // WIN32: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+  // WIN32: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+
+  // CHECK: VarDecl {{.*}} cdecl '
+  // CHECK: LambdaExpr
+  // CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+  // NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+  // NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+  // VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+  // VECTDEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+  // CHECK: VarDecl {{.*}} genericlambda '
+  // CHECK: LambdaExpr
+  //
+  // CHECK: FunctionTemplateDecl {{.*}} operator()
+  // LIN64: CXXMethodDecl {{.*}} operator() 'auto (auto) const' inline
+  // LIN64: CXXMethodDecl {{.*}} operator() 'void (char) const' inline
+  // LIN64: CXXMethodDecl {{.*}} 

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a subscriber: majnemer.
erichkeane added a comment.

Turns out this patch: 
https://github.com/llvm/llvm-project/commit/2e1e0491b7098fcfe01945e8f62cafe1fcb3cf36
 is my problem. The issue has to do deducing a 'local' type in the return type. 
 As a result, we choose to mangle any type 'containing' auto as 'auto'.

So ours demangles as: ##public: __thiscall `void __cdecl 
usage(void)'::`1'operator (void)const ##
MSVC of course demangles as: ##public: __thiscall 
::operator double 
(__cdecl*)(int,float,double)(void)const ##

To me, it seems that the ResultType->getContainedAutoType is perhaps in the 
wrong here (also of interest, the dyn_cast, for a function that already returns 
AutoType?)? I'm not sure how to workaround this.  I can of course create an 
awkward reproducer of this (https://godbolt.org/z/KWPTTh), but I don't have a 
good idea on how to better handle this.  @majnemer I see you were the initial 
committer of this, and would like to see if you have any ideas?


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D89559#2347642 , @rjmccall wrote:

> Mangling more calling conventions when mangling function types in Itanium 
> (except at the top level) is the right thing to do.  There's already a place 
> to do so in the mangling.  We just haven't done this yet because a lot of 
> those calling convention are supported by other compilers, so we need to 
> coordinate.  So you need to check out what other compilers do (GCC, ICC) and 
> imitate them if they're already setting a reasonable default; otherwise, I 
> think there's a standard algorithm for generating these.
>
> Separately, the MSVC mangler should support Clang's CCs if there's a 
> reasonable extensible rule there.  I've never been able to figure out the 
> MVSC mangling grammar.

Same here :)

I remember there being places to mangle calling convention, I was just 
surprised they didn't happen in the function type here.  GCC doesn't seem to 
support any of the calling conventions in 64 bit, but I'll see if ICC has 
selected a mangling scheme.


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Mangling more calling conventions when mangling function types in Itanium 
(except at the top level) is the right thing to do.  There's already a place to 
do so in the mangling.  We just haven't done this yet because a lot of those 
calling convention are supported by other compilers, so we need to coordinate.  
So you need to check out what other compilers do (GCC, ICC) and imitate them if 
they're already setting a reasonable default; otherwise, I think there's a 
standard algorithm for generating these.

Separately, the MSVC mangler should support Clang's CCs if there's a reasonable 
extensible rule there.  I've never been able to figure out the MVSC mangling 
grammar.


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Hmm... so I got distracted the last few days with a handful of small SEMA 
issues that I believe to be solved, so I'm going back to my codegen issues.

It seems that my problem is that we don't actually mangle calling-convention in 
a pointer types.  The result is:
64 bit: https://godbolt.org/z/nhnoG9
32 bit: https://godbolt.org/z/eK6jha

As you can see, the, only 32 bit-windows actually gets that right in that it 
differentiates between the two calling-conventions on the operator-func-ptr.  
The result is the other 3 platforms all get the AST right, but don't generate 
separate implementations for them.  MY implementation is still wrong, since 
something else odd happens with lambdas on 32 bits, but I'd like to solve these 
problems as well.

I'll work on the Lambda issue to at least get windows 32 bit right (the only 
place that will have multiple operator-func-ptr in this case), but I'll 
eventually need to solve the issue with the differing calling conventions for 
the MSVC compat implementation that is a follow up to this. Any suggestions on 
where that could fit in?


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D89559#2340357 , @rsmith wrote:

> In D89559#2339206 , @erichkeane 
> wrote:
>
>> Since the work is 99% the same, I think I should start doing that (emitting 
>> BOTH in the case where it matches the default member but NOT the default 
>> free function CC) in this review, then do a separate commit for the MSVC 
>> compat mode where we emit a version for ALL of the calling conventions.
>
> You'll also need an overload resolution tiebreaker to ensure that `+[]{}` and 
> the like are not ill-formed due to ambiguity.

Right, thats a good point.  I'll make sure that is one of my tests.  Thanks!


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D89559#2339206 , @erichkeane wrote:

> Since the work is 99% the same, I think I should start doing that (emitting 
> BOTH in the case where it matches the default member but NOT the default free 
> function CC) in this review, then do a separate commit for the MSVC compat 
> mode where we emit a version for ALL of the calling conventions.

You'll also need an overload resolution tiebreaker to ensure that `+[]{}` and 
the like are not ill-formed due to ambiguity.


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D89559#2339192 , @rjmccall wrote:

> Yeah, it might be reasonable to just do that on targets where the defaults 
> are different (just MSVC, I think?) and otherwise preserve the method's 
> calling convention.

I think it is actually just MSVC-32 bit!

Since the work is 99% the same, I think I should start doing that (emitting 
BOTH in the case where it matches the default member but NOT the default free 
function CC) in this review, then do a separate commit for the MSVC compat mode 
where we emit a version for ALL of the calling conventions.

Thanks for the feedback!


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Yeah, it might be reasonable to just do that on targets where the defaults are 
different (just MSVC, I think?) and otherwise preserve the method's calling 
convention.


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D89559#2336335 , @rjmccall wrote:

> We can't have it always match, that would require us to reject the conversion 
> to a bare function-pointer type, which is required by the standard.
>
> I'm not sure if MSVC's multiple conversions hack is conformant or not, but 
> it's at least more conformant than that.

Right, I should have remembered that.  Though, I guess that brings up the 
alternative, which is to simply produce ALL of the conversion/invoke functions 
like MSVC.  I was originally attempting to implement that (under a MSVCCompat 
flag) and decided fixing this first would be an easier way to go, but I could 
just continue with that implementation for ALL valid CCs on all platforms.

It is slightly more difficult than just this I believe, as there is some 
codegen error that I need to fix, but otherwise it should be fine.


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

We can't have it always match, that would require us to reject the conversion 
to a bare function-pointer type, which is required by the standard.

I'm not sure if MSVC's multiple conversions hack is conformant or not, but it's 
at least more conformant than that.


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaLambda.cpp:1278
+  if (CallOpCC == DefaultMember)
+return DefaultFree;
+  return CallOpCC;

rsmith wrote:
> rsmith wrote:
> > erichkeane wrote:
> > > rjmccall wrote:
> > > > ...I made this comment in my first review, but Phabricator threw it 
> > > > away.
> > > > 
> > > > The attributes let you explicitly request the default method CC, right? 
> > > >  I think you need to check for an explicit attribute rather than just 
> > > > checking CC identity.  There should be an AttributedType in the sugar.
> > > They do, but I can't seem to find a way to find them.  The calling 
> > > convention is already merged into the functiontype by the time we get 
> > > here, the AttributedType isn't accessible.
> > > 
> > > So it seems we don't distinguish between "modified by attribute", 
> > > "modified-default by command line", and "modified-default by TargetInfo."
> > > 
> > > That said, I somewhat think this is the right thing to do anyway.  If 
> > > you're on a platform where the default call convention is different 
> > > between a free-function and member-function, I'd think that this is what 
> > > you MEAN...
> > The `AttributedType` should be present in the type on the `TypeSourceInfo` 
> > for the call operator. It might not be present on the type retrieved by 
> > `getType()`, though.
> > 
> > Concretely, what targets have different calling conventions for member 
> > versus non-member functions, and what do those calling conventions do 
> > differently? (For example, if the default member calling convention treats 
> > `this` differently, it doesn't seem reasonable to apply that to the static 
> > invoker...)
> Answering my own question: the only case where the default member calling 
> convention is different from the default non-member calling convention is for 
> MinGW on 32-bit x86, where members use `thiscall` by default (which passes 
> the first parameter in `%ecx`).
> 
> Is it reasonable for `[] [[clang::thiscall]] {}` to result in a static 
> invoker using the `thiscall` calling convention? Intuitively I'd say no, but 
> there's not really anything specific to member functions in `thiscall` -- it 
> just means that we pass the first argument in a register. (However, the first 
> argument of the lambda and the first argument of its static invoker are 
> different, so it's still somewhat unclear whether inheriting `thiscall` is 
> appropriate. But usually for any given lambda only one of the two is actually 
> used.)
> 
> But there's another quirk here: the default non-member calling convention can 
> be set on the command line with `-fdefault-calling-conv=` (and a few other 
> flags such as `-mrtd`). This doesn't affect member functions by default. So 
> what should happen if this is compiled with `-fdefault-calling-conv=stdcall` 
> (assuming the default calling convention is otherwise `cdecl`):
> 
> ```
> auto *p0 = [] [[clang::stdcal]] {};
> auto *p1 = [] {};
> auto *p2 = [] [[clang::cdecl]] {};
> ```
> 
> `p0` seems easy: the default non-member calling convention and the explicit 
> calling convention are the same. The invoker should be `stdcall`.
> 
> For `p1`, the default member calling convention is `cdecl` but the default 
> non-member calling convention is `stdcall`. In this case, conformance 
> requires us to use `stdcall` for the pointer type, because `p1` is required 
> to have type `void (*)()`, which is a `stdcall` function pointer in this 
> configuration.
> 
> For `p2`, the call operator's calling convention has been explicitly set to 
> the default member calling convention. I think in this case I'd expect a 
> `cdecl` static invoker.
> 
> So I think I'm inclined to say we should always apply an explicit calling 
> convention to both the call operator and the static invoker -- that seems 
> like the simplest and easiest to explain rule, and probably matches user 
> expectations most of the time, especially given the observation that you're 
> usually writing a lambda only for one or the other of the call operator and 
> the static invoker, so if you explicitly write a calling convention 
> attribute, you presumably want it to apply to the part of the lambda's 
> interface that you're using.
> Answering my own question: the only case where the default member calling 
> convention is different from the default non-member calling convention is for 
> MinGW on 32-bit x86, where members use `thiscall` by default (which passes 
> the first parameter in `%ecx`).
> 
> Is it reasonable for `[] [[clang::thiscall]] {}` to result in a static 
> invoker using the `thiscall` calling convention? Intuitively I'd say no, but 
> there's not really anything specific to member functions in `thiscall` -- it 
> just means that we pass the first argument in a register. (However, the first 
> argument of the lambda and the first argument of its static invoker are 
> different, so it's still somewhat 

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaLambda.cpp:1278
+  if (CallOpCC == DefaultMember)
+return DefaultFree;
+  return CallOpCC;

rsmith wrote:
> erichkeane wrote:
> > rjmccall wrote:
> > > ...I made this comment in my first review, but Phabricator threw it away.
> > > 
> > > The attributes let you explicitly request the default method CC, right?  
> > > I think you need to check for an explicit attribute rather than just 
> > > checking CC identity.  There should be an AttributedType in the sugar.
> > They do, but I can't seem to find a way to find them.  The calling 
> > convention is already merged into the functiontype by the time we get here, 
> > the AttributedType isn't accessible.
> > 
> > So it seems we don't distinguish between "modified by attribute", 
> > "modified-default by command line", and "modified-default by TargetInfo."
> > 
> > That said, I somewhat think this is the right thing to do anyway.  If 
> > you're on a platform where the default call convention is different between 
> > a free-function and member-function, I'd think that this is what you MEAN...
> The `AttributedType` should be present in the type on the `TypeSourceInfo` 
> for the call operator. It might not be present on the type retrieved by 
> `getType()`, though.
> 
> Concretely, what targets have different calling conventions for member versus 
> non-member functions, and what do those calling conventions do differently? 
> (For example, if the default member calling convention treats `this` 
> differently, it doesn't seem reasonable to apply that to the static 
> invoker...)
Answering my own question: the only case where the default member calling 
convention is different from the default non-member calling convention is for 
MinGW on 32-bit x86, where members use `thiscall` by default (which passes the 
first parameter in `%ecx`).

Is it reasonable for `[] [[clang::thiscall]] {}` to result in a static invoker 
using the `thiscall` calling convention? Intuitively I'd say no, but there's 
not really anything specific to member functions in `thiscall` -- it just means 
that we pass the first argument in a register. (However, the first argument of 
the lambda and the first argument of its static invoker are different, so it's 
still somewhat unclear whether inheriting `thiscall` is appropriate. But 
usually for any given lambda only one of the two is actually used.)

But there's another quirk here: the default non-member calling convention can 
be set on the command line with `-fdefault-calling-conv=` (and a few other 
flags such as `-mrtd`). This doesn't affect member functions by default. So 
what should happen if this is compiled with `-fdefault-calling-conv=stdcall` 
(assuming the default calling convention is otherwise `cdecl`):

```
auto *p0 = [] [[clang::stdcal]] {};
auto *p1 = [] {};
auto *p2 = [] [[clang::cdecl]] {};
```

`p0` seems easy: the default non-member calling convention and the explicit 
calling convention are the same. The invoker should be `stdcall`.

For `p1`, the default member calling convention is `cdecl` but the default 
non-member calling convention is `stdcall`. In this case, conformance requires 
us to use `stdcall` for the pointer type, because `p1` is required to have type 
`void (*)()`, which is a `stdcall` function pointer in this configuration.

For `p2`, the call operator's calling convention has been explicitly set to the 
default member calling convention. I think in this case I'd expect a `cdecl` 
static invoker.

So I think I'm inclined to say we should always apply an explicit calling 
convention to both the call operator and the static invoker -- that seems like 
the simplest and easiest to explain rule, and probably matches user 
expectations most of the time, especially given the observation that you're 
usually writing a lambda only for one or the other of the call operator and the 
static invoker, so if you explicitly write a calling convention attribute, you 
presumably want it to apply to the part of the lambda's interface that you're 
using.


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaLambda.cpp:1278
+  if (CallOpCC == DefaultMember)
+return DefaultFree;
+  return CallOpCC;

erichkeane wrote:
> rjmccall wrote:
> > ...I made this comment in my first review, but Phabricator threw it away.
> > 
> > The attributes let you explicitly request the default method CC, right?  I 
> > think you need to check for an explicit attribute rather than just checking 
> > CC identity.  There should be an AttributedType in the sugar.
> They do, but I can't seem to find a way to find them.  The calling convention 
> is already merged into the functiontype by the time we get here, the 
> AttributedType isn't accessible.
> 
> So it seems we don't distinguish between "modified by attribute", 
> "modified-default by command line", and "modified-default by TargetInfo."
> 
> That said, I somewhat think this is the right thing to do anyway.  If you're 
> on a platform where the default call convention is different between a 
> free-function and member-function, I'd think that this is what you MEAN...
The `AttributedType` should be present in the type on the `TypeSourceInfo` for 
the call operator. It might not be present on the type retrieved by 
`getType()`, though.

Concretely, what targets have different calling conventions for member versus 
non-member functions, and what do those calling conventions do differently? 
(For example, if the default member calling convention treats `this` 
differently, it doesn't seem reasonable to apply that to the static invoker...)


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Perhaps a better example:

(gdb) p CallOperator->getType()->dump()
FunctionProtoType 0x1177edf0 'void (void) __attribute__((vectorcall)) const' 
const vectorcall
`-AutoType 0x1177a0d0 'void' sugar

  `-BuiltinType 0x11716dc0 'void'


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D89559#2335557 , @rjmccall wrote:

> No, if you put a calling convention on a lambda and then convert it to a 
> function pointer, you almost certainly want that CC to be honored.
>
> Is the `AttributedType` still present in `CallOperator->getType()`?

No, it is not at that point:

(gdb) p CallOperator->getType()->dump()
FunctionProtoType 0x1177a160 'void (void) __attribute__((thiscall)) const' 
const thiscall
`-AutoType 0x1177a0d0 'void' sugar

  `-BuiltinType 0x11716dc0 'void'


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

No, if you put a calling convention on a lambda and then convert it to a 
function pointer, you almost certainly want that CC to be honored.

Is the `AttributedType` still present in `CallOperator->getType()`?


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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 298666.
erichkeane marked an inline comment as done.
erichkeane added a comment.

Calc->get.


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

https://reviews.llvm.org/D89559

Files:
  clang/lib/Sema/SemaLambda.cpp
  clang/test/SemaCXX/lambda-conversion-op-cc.cpp

Index: clang/test/SemaCXX/lambda-conversion-op-cc.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-conversion-op-cc.cpp
@@ -0,0 +1,79 @@
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc %s -verify -DBAD_CONVERSION
+// RUN: %clang_cc1 -fsyntax-only -triple i386-windows-pc %s -verify -DBAD_CONVERSION -DWIN32
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc %s -ast-dump | FileCheck %s --check-prefixes=CHECK,LIN64,NODEF
+// RUN: %clang_cc1 -fsyntax-only -triple i386-windows-pc %s -ast-dump -DWIN32 | FileCheck %s --check-prefixes=CHECK,WIN32,NODEF
+
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc -fdefault-calling-conv=vectorcall %s -verify -DBAD_VEC_CONVERS
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc -fdefault-calling-conv=vectorcall %s -ast-dump | FileCheck %s --check-prefixes=CHECK,VECTDEF
+
+void useage() {
+  auto normal = [](int, float, double){}; // #1
+  auto vectorcall = [](int, float, double) __attribute__((vectorcall)) {}; // #2
+#ifdef WIN32
+  auto thiscall = [](int, float, double) __attribute__((thiscall)) {}; // #3
+#endif // WIN32
+  auto cdecl = [](int, float, double) __attribute__((cdecl)) {};
+
+// CHECK: VarDecl {{.*}} normal '
+// CHECK: LambdaExpr
+// WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+// LIN64: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+// VECTDEF: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+// NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+// NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+// VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+// VECTDEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+// CHECK: VarDecl {{.*}} vectorcall '
+// CHECK: LambdaExpr
+// CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((vectorcall)) const'
+// CHECK: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+// CHECK: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+// WIN32: VarDecl {{.*}} thiscall '
+// WIN32: LambdaExpr
+// WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+// WIN32: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+// WIN32: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+
+// CHECK: VarDecl {{.*}} cdecl '
+// CHECK: LambdaExpr
+// CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+// NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+// NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+// VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+// VECTDEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+#ifdef BAD_CONVERSION
+  // expected-error-re@+2 {{no viable conversion from {{.*}} to 'void (*)(int, float, double) __attribute__((vectorcall))}}
+  // expected-note@#1 {{candidate function}}
+  void (*__attribute__((vectorcall)) normal_ptr2)(int, float, double) = normal;
+  // expected-error-re@+2 {{no viable conversion from {{.*}} to 'void (*)(int, float, double)}}
+  // expected-note@#2 {{candidate function}}
+  void (*vectorcall_ptr2)(int, float, double) = vectorcall;
+#ifdef WIN32
+  // expected-error-re@+2 {{no viable conversion from {{.*}} to 'void (*)(int, float, double) __attribute__((thiscall))}}
+  // expected-note@#3 {{candidate function}}
+  void (*__attribute__((thiscall)) thiscall_ptr2)(int, float, double) = thiscall;
+#endif // WIN32
+#endif // BAD_CONVERSION
+
+#ifdef BAD_VEC_CONVERS
+  void (*__attribute__((vectorcall)) normal_ptr2)(int, float, double) = normal;
+  void (* normal_ptr3)(int, float, double) = normal;
+  // expected-error-re@+2 {{no viable conversion from {{.*}} to 'void (*)(int, float, double) __attribute__((regcall))}}
+  // expected-note@#1 {{candidate function}}
+  void (*__attribute__((regcall)) normalptr4)(int, float, double) = normal;
+  void (*__attribute__((vectorcall)) vectorcall_ptr2)(int, float, double) = vectorcall;
+  void (* vectorcall_ptr3)(int, float, double) = vectorcall;
+#endif // BAD_VEC_CONVERS
+
+  // Required to force emission of the invoker.
+  void (*normal_ptr)(int, float, double) = normal;
+  void 

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaLambda.cpp:1278
+  if (CallOpCC == DefaultMember)
+return DefaultFree;
+  return CallOpCC;

rjmccall wrote:
> ...I made this comment in my first review, but Phabricator threw it away.
> 
> The attributes let you explicitly request the default method CC, right?  I 
> think you need to check for an explicit attribute rather than just checking 
> CC identity.  There should be an AttributedType in the sugar.
They do, but I can't seem to find a way to find them.  The calling convention 
is already merged into the functiontype by the time we get here, the 
AttributedType isn't accessible.

So it seems we don't distinguish between "modified by attribute", 
"modified-default by command line", and "modified-default by TargetInfo."

That said, I somewhat think this is the right thing to do anyway.  If you're on 
a platform where the default call convention is different between a 
free-function and member-function, I'd think that this is what you MEAN...


Repository:
  rC Clang

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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked an inline comment as done.
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaLambda.cpp:1268
+calcLambdaConversionFunctionCallConv(Sema ,
+ const FunctionProtoType *CallOpProto) {
+  CallingConv DefaultFree = S.Context.getDefaultCallingConvention(

rjmccall wrote:
> I don't think we use `calc` as a prefix anywhere else in the compiler.  Maybe 
> `get`?
WFM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaLambda.cpp:1278
+  if (CallOpCC == DefaultMember)
+return DefaultFree;
+  return CallOpCC;

...I made this comment in my first review, but Phabricator threw it away.

The attributes let you explicitly request the default method CC, right?  I 
think you need to check for an explicit attribute rather than just checking CC 
identity.  There should be an AttributedType in the sugar.


Repository:
  rC Clang

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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I agree that it seems reasonable to preserve an explicit CC from the lambda on 
the converted function pointer.




Comment at: clang/lib/Sema/SemaLambda.cpp:1268
+calcLambdaConversionFunctionCallConv(Sema ,
+ const FunctionProtoType *CallOpProto) {
+  CallingConv DefaultFree = S.Context.getDefaultCallingConvention(

I don't think we use `calc` as a prefix anywhere else in the compiler.  Maybe 
`get`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D89559

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[Erich Keane via Phabricator via cfe-commits]

erichkeane created this revision.
erichkeane added reviewers: rsmith, rjmccall.
erichkeane requested review of this revision.

As mentioned in the defect, the lambda static invoker does not follow
the calling convention of the lambda itself, which seems wrong. This
patch ensures that the calling convention of operator() is passed onto
the invoker and conversion-operator type.

This is accomplished by extracting the calling-convention determination
code out into a separate function in order to better reflect the 'thiscall'
work, as well as somewhat better support the future implementation of 
https://devblogs.microsoft.com/oldnewthing/20150220-00/?p=44623


Repository:
  rC Clang

https://reviews.llvm.org/D89559

Files:
  clang/lib/Sema/SemaLambda.cpp
  clang/test/SemaCXX/lambda-conversion-op-cc.cpp

Index: clang/test/SemaCXX/lambda-conversion-op-cc.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/lambda-conversion-op-cc.cpp
@@ -0,0 +1,79 @@
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc %s -verify -DBAD_CONVERSION
+// RUN: %clang_cc1 -fsyntax-only -triple i386-windows-pc %s -verify -DBAD_CONVERSION -DWIN32
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc %s -ast-dump | FileCheck %s --check-prefixes=CHECK,LIN64,NODEF
+// RUN: %clang_cc1 -fsyntax-only -triple i386-windows-pc %s -ast-dump -DWIN32 | FileCheck %s --check-prefixes=CHECK,WIN32,NODEF
+
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc -fdefault-calling-conv=vectorcall %s -verify -DBAD_VEC_CONVERS
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-pc -fdefault-calling-conv=vectorcall %s -ast-dump | FileCheck %s --check-prefixes=CHECK,VECTDEF
+
+void useage() {
+  auto normal = [](int, float, double){}; // #1
+  auto vectorcall = [](int, float, double) __attribute__((vectorcall)) {}; // #2
+#ifdef WIN32
+  auto thiscall = [](int, float, double) __attribute__((thiscall)) {}; // #3
+#endif // WIN32
+  auto cdecl = [](int, float, double) __attribute__((cdecl)) {};
+
+// CHECK: VarDecl {{.*}} normal '
+// CHECK: LambdaExpr
+// WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+// LIN64: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+// VECTDEF: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+// NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+// NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+// VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+// VECTDEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+// CHECK: VarDecl {{.*}} vectorcall '
+// CHECK: LambdaExpr
+// CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((vectorcall)) const'
+// CHECK: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+// CHECK: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+// WIN32: VarDecl {{.*}} thiscall '
+// WIN32: LambdaExpr
+// WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+// WIN32: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+// WIN32: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+
+// CHECK: VarDecl {{.*}} cdecl '
+// CHECK: LambdaExpr
+// CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+// NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
+// NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
+// VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
+// VECTDEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
+
+#ifdef BAD_CONVERSION
+  // expected-error-re@+2 {{no viable conversion from {{.*}} to 'void (*)(int, float, double) __attribute__((vectorcall))}}
+  // expected-note@#1 {{candidate function}}
+  void (*__attribute__((vectorcall)) normal_ptr2)(int, float, double) = normal;
+  // expected-error-re@+2 {{no viable conversion from {{.*}} to 'void (*)(int, float, double)}}
+  // expected-note@#2 {{candidate function}}
+  void (*vectorcall_ptr2)(int, float, double) = vectorcall;
+#ifdef WIN32
+  // expected-error-re@+2 {{no viable conversion from {{.*}} to 'void (*)(int, float, double) __attribute__((thiscall))}}
+  // expected-note@#3 {{candidate function}}
+  void (*__attribute__((thiscall)) thiscall_ptr2)(int, float, double) = thiscall;
+#endif // WIN32
+#endif // BAD_CONVERSION
+
+#ifdef BAD_VEC_CONVERS
+  void (*__attribute__((vectorcall)) normal_ptr2)(int, float, double) = normal;
+  void (* normal_ptr3)(int, float, double) = normal;
+  // expected-error-re@+2 {{no viable conversion from {{.*}} to