[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Alright then, I've disabled by default(911add149af563a5a61458de0dd730e3f5348623 
). I'll 
enable 1 at a time as I fix the calling conventions in D79118 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

See my audit/WIP on calling conventions here: https://reviews.llvm.org/D79118


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D78785#2010738 , @erichkeane wrote:

> In D78785#2010654 , @rjmccall wrote:
>
> > Every target does something similar, they just all do it in different ways 
> > because they're mostly written by different people.
> >
> > We should restrict this feature to targets where we've adequately audited 
> > the ABI.  It's not a feature until the ABI work is done.
>
>
> I'm discovering that now :)  I'm currently trying to do said audit by going 
> down the ABIInfo inheritance tree.  I believe we have at least a month until 
> the next release branch date.  If you can help keep me honest here, I'll 
> disable _ExtInt for any platforms I haven't done by then at the branch.
>
> Does that seem acceptable?


Yes, although, I think you ought to flip the default anyway just so we're doing 
the right thing for out-of-tree targets.  It should then be very 
straightforward to flip it back target-by-target as you do your audit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D78785#2010654 , @rjmccall wrote:

> Every target does something similar, they just all do it in different ways 
> because they're mostly written by different people.
>
> We should restrict this feature to targets where we've adequately audited the 
> ABI.  It's not a feature until the ABI work is done.


I'm discovering that now :)  I'm currently trying to do said audit by going 
down the ABIInfo inheritance tree.  I believe we have at least a month until 
the next release branch date.  If you can help keep me honest here, I'll 
disable _ExtInt for any platforms I haven't done by then at the branch.

Does that seem acceptable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Every target does something similar, they just all do it in different ways 
because they're mostly written by different people.

We should restrict this feature to targets where we've adequately audited the 
ABI.  It's not a feature until the ABI work is done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5a1d9c0f5ac8: Fix x86/x86_64 calling convention for _ExtInt 
(authored by erichkeane).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78785

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/ext-int-sanitizer.cpp
  clang/test/CodeGen/ext-int.c
  clang/test/CodeGenCXX/ext-int.cpp

Index: clang/test/CodeGenCXX/ext-int.cpp
===
--- clang/test/CodeGenCXX/ext-int.cpp
+++ clang/test/CodeGenCXX/ext-int.cpp
@@ -98,7 +98,7 @@
 };
 
 void UnderlyingTypeUsage(AsEnumUnderlyingType Param) {
-  // LIN: define void @_Z19UnderlyingTypeUsage20AsEnumUnderlyingType(i9 %
+  // LIN: define void @_Z19UnderlyingTypeUsage20AsEnumUnderlyingType(i16 %
   // WIN: define dso_local void @"?UnderlyingTypeUsage@@YAXW4AsEnumUnderlyingType@@@Z"(i9 %
   AsEnumUnderlyingType Var;
   // CHECK: alloca i9, align 2
@@ -106,13 +106,13 @@
 }
 
 unsigned _ExtInt(33) ManglingTestRetParam(unsigned _ExtInt(33) Param) {
-// LIN: define i33 @_Z20ManglingTestRetParamU7_ExtIntILi33EEj(i33 %
+// LIN: define i64 @_Z20ManglingTestRetParamU7_ExtIntILi33EEj(i64 %
 // WIN: define dso_local i33 @"?ManglingTestRetParam@@YAU?$_UExtInt@$0CB@@__clang@@U12@@Z"(i33
   return 0;
 }
 
 _ExtInt(33) ManglingTestRetParam(_ExtInt(33) Param) {
-// LIN: define i33 @_Z20ManglingTestRetParamU7_ExtIntILi33EEi(i33 %
+// LIN: define i64 @_Z20ManglingTestRetParamU7_ExtIntILi33EEi(i64 %
 // WIN: define dso_local i33 @"?ManglingTestRetParam@@YAU?$_ExtInt@$0CB@@__clang@@U12@@Z"(i33
   return 0;
 }
@@ -155,13 +155,14 @@
 
   _ExtInt(92) A = __builtin_va_arg(args, _ExtInt(92));
   // LIN: %[[AD1:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
-  // LIN: %[[OFA_P1:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD1]], i32 0, i32 2
-  // LIN: %[[OFA1:.+]] = load i8*, i8** %[[OFA_P1]]
-  // LIN: %[[BC1:.+]] = bitcast i8* %[[OFA1]] to i92*
-  // LIN: %[[OFANEXT1:.+]] = getelementptr i8, i8* %[[OFA1]], i32 16
-  // LIN: store i8* %[[OFANEXT1]], i8** %[[OFA_P1]]
+  // LIN: %[[OFA_P1:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD1]], i32 0, i32 0
+  // LIN: %[[GPOFFSET:.+]] = load i32, i32* %[[OFA_P1]]
+  // LIN: %[[FITSINGP:.+]] = icmp ule i32 %[[GPOFFSET]], 32
+  // LIN: br i1 %[[FITSINGP]]
+  // LIN: %[[BC1:.+]] = phi i92*
   // LIN: %[[LOAD1:.+]] = load i92, i92* %[[BC1]]
   // LIN: store i92 %[[LOAD1]], i92*
+
   // WIN: %[[CUR1:.+]] = load i8*, i8** %[[ARGS]]
   // WIN: %[[NEXT1:.+]] = getelementptr inbounds i8, i8* %[[CUR1]], i64 16
   // WIN: store i8* %[[NEXT1]], i8** %[[ARGS]]
@@ -171,15 +172,16 @@
 
   _ExtInt(31) B = __builtin_va_arg(args, _ExtInt(31));
   // LIN: %[[AD2:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
-  // LIN: %[[OFA_P2:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD2]], i32 0, i32 2
-  // LIN: %[[OFA2:.+]] = load i8*, i8** %[[OFA_P2]]
-  // LIN: %[[BC2:.+]] = bitcast i8* %[[OFA2]] to i31*
-  // LIN: %[[OFANEXT2:.+]] = getelementptr i8, i8* %[[OFA2]], i32 8
-  // LIN: store i8* %[[OFANEXT2]], i8** %[[OFA_P2]]
-  // LIN: %[[LOAD2:.+]] = load i31, i31* %[[BC2]]
-  // LIN: store i31 %[[LOAD2]], i31*
+  // LIN: %[[OFA_P2:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD2]], i32 0, i32 0
+  // LIN: %[[GPOFFSET:.+]] = load i32, i32* %[[OFA_P2]]
+  // LIN: %[[FITSINGP:.+]] = icmp ule i32 %[[GPOFFSET]], 40
+  // LIN: br i1 %[[FITSINGP]]
+  // LIN: %[[BC1:.+]] = phi i31*
+  // LIN: %[[LOAD1:.+]] = load i31, i31* %[[BC1]]
+  // LIN: store i31 %[[LOAD1]], i31*
+
   // WIN: %[[CUR2:.+]] = load i8*, i8** %[[ARGS]]
-  // WIN: %[[NEXT2:.+]] = getelementptr inbounds i8, i8* %[[CUR2]], i64 8 
+  // WIN: %[[NEXT2:.+]] = getelementptr inbounds i8, i8* %[[CUR2]], i64 8
   // WIN: store i8* %[[NEXT2]], i8** %[[ARGS]]
   // WIN: %[[BC2:.+]] = bitcast i8* %[[CUR2]] to i31*
   // WIN: %[[LOADV2:.+]] = load i31, i31* %[[BC2]]
@@ -187,13 +189,14 @@
 
   _ExtInt(16) C = __builtin_va_arg(args, _ExtInt(16));
   // LIN: %[[AD3:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
-  // LIN: %[[OFA_P3:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD3]], i32 0, i32 2
-  // LIN: %[[OFA3:.+]] = load i8*, i8** %[[OFA_P3]]
-  // LIN: %[[BC3:.+]] = bitcast i8* %[[OFA3]] to i16*
-  // LIN: %[[OFANEXT3:.+]] = getelementptr i8, i8* %[[OFA3]], i32 8
-  // LIN: store i8* %[[OFANEXT3]], i8** %[[OFA_P3]]
-  // LIN: %[[LOAD3:.+]] = load i16, i16* %[[BC3]]
-  // LIN: store i16 %[[LOAD3]], i16*
+  // LIN: %[[OFA_P3:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD3]], i32 0, i32 0
+  // LIN: %[

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D78785#2010546 , @rjmccall wrote:

> Could you answer my question up-thread about whether ExtInt is currently 
> target-limited?  If it isn't, we need to more broadly audit targets.


Yikes!  I missed that question, I'm sorry :/

We aren't limiting it, as it is supposed to be a C language feature (and in 
fact, is more useful certain on non-x86 chips!).  We likely need an audit, 
though in TargetInfo I didn't see any other 'classify' functions other than 
these 3 (X86, x86-64Win, x86-64-Linux).


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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Could you answer my question up-thread about whether ExtInt is currently 
target-limited?  If it isn't, we need to more broadly audit targets.


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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Craig Topper via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

@craig.topper Did this look OK/  Is there anything else you want from this?


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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/TargetInfo.cpp:2980
 
-return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty)
-  : ABIArgInfo::getDirect());
+if (!Ty->isExtIntType())
+  return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty)

rjmccall wrote:
> erichkeane wrote:
> > rjmccall wrote:
> > > Presumably *some* `ExtInt` types should be returned directly.
> > Right, and they are.  This is only for cases where we request an 'indirect 
> > result', so cases where we've already decided that it cannot be passed 
> > direct.
> > 
> > At this point, the 'classify' step has already been done, which would have 
> > decided that <=128 bit values are passed 'direct', and we've requested an 
> > indirect.  At least as far as I can tell from this code/debugging.
> Oh, I see.  So the current code is intentionally using a direct IR return in 
> some cases and then allowing LLVM to lower it as an indirect return?  I 
> didn't think we ever did that, and I'm really not sure it's a good idea, so I 
> definitely agree with not doing it for ExtInt types.
Right, exactly :)  FWIW, this is my 3rd time looking into this TargetInfo code, 
and it is particularly convoluted. If I understood the calling conventions 
sufficiently, I'd take a run at trying to figure out how to make this not so 
messy.


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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:2980
 
-return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty)
-  : ABIArgInfo::getDirect());
+if (!Ty->isExtIntType())
+  return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty)

erichkeane wrote:
> rjmccall wrote:
> > Presumably *some* `ExtInt` types should be returned directly.
> Right, and they are.  This is only for cases where we request an 'indirect 
> result', so cases where we've already decided that it cannot be passed direct.
> 
> At this point, the 'classify' step has already been done, which would have 
> decided that <=128 bit values are passed 'direct', and we've requested an 
> indirect.  At least as far as I can tell from this code/debugging.
Oh, I see.  So the current code is intentionally using a direct IR return in 
some cases and then allowing LLVM to lower it as an indirect return?  I didn't 
think we ever did that, and I'm really not sure it's a good idea, so I 
definitely agree with not doing it for ExtInt types.


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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/TargetInfo.cpp:2980
 
-return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty)
-  : ABIArgInfo::getDirect());
+if (!Ty->isExtIntType())
+  return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty)

rjmccall wrote:
> Presumably *some* `ExtInt` types should be returned directly.
Right, and they are.  This is only for cases where we request an 'indirect 
result', so cases where we've already decided that it cannot be passed direct.

At this point, the 'classify' step has already been done, which would have 
decided that <=128 bit values are passed 'direct', and we've requested an 
indirect.  At least as far as I can tell from this code/debugging.


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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Is there something currently restricting these types to just x86, or do you 
need to do a much broader audit?




Comment at: clang/lib/CodeGen/TargetInfo.cpp:2980
 
-return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty)
-  : ABIArgInfo::getDirect());
+if (!Ty->isExtIntType())
+  return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty)

Presumably *some* `ExtInt` types should be returned directly.


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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

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

Handle indirect return as well.  @craig.topper : Anything else you can think of?


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

https://reviews.llvm.org/D78785

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/ext-int-sanitizer.cpp
  clang/test/CodeGen/ext-int.c
  clang/test/CodeGenCXX/ext-int.cpp

Index: clang/test/CodeGenCXX/ext-int.cpp
===
--- clang/test/CodeGenCXX/ext-int.cpp
+++ clang/test/CodeGenCXX/ext-int.cpp
@@ -98,7 +98,7 @@
 };
 
 void UnderlyingTypeUsage(AsEnumUnderlyingType Param) {
-  // LIN: define void @_Z19UnderlyingTypeUsage20AsEnumUnderlyingType(i9 %
+  // LIN: define void @_Z19UnderlyingTypeUsage20AsEnumUnderlyingType(i16 %
   // WIN: define dso_local void @"?UnderlyingTypeUsage@@YAXW4AsEnumUnderlyingType@@@Z"(i9 %
   AsEnumUnderlyingType Var;
   // CHECK: alloca i9, align 2
@@ -106,13 +106,13 @@
 }
 
 unsigned _ExtInt(33) ManglingTestRetParam(unsigned _ExtInt(33) Param) {
-// LIN: define i33 @_Z20ManglingTestRetParamU7_ExtIntILi33EEj(i33 %
+// LIN: define i64 @_Z20ManglingTestRetParamU7_ExtIntILi33EEj(i64 %
 // WIN: define dso_local i33 @"?ManglingTestRetParam@@YAU?$_UExtInt@$0CB@@__clang@@U12@@Z"(i33
   return 0;
 }
 
 _ExtInt(33) ManglingTestRetParam(_ExtInt(33) Param) {
-// LIN: define i33 @_Z20ManglingTestRetParamU7_ExtIntILi33EEi(i33 %
+// LIN: define i64 @_Z20ManglingTestRetParamU7_ExtIntILi33EEi(i64 %
 // WIN: define dso_local i33 @"?ManglingTestRetParam@@YAU?$_ExtInt@$0CB@@__clang@@U12@@Z"(i33
   return 0;
 }
@@ -155,13 +155,14 @@
 
   _ExtInt(92) A = __builtin_va_arg(args, _ExtInt(92));
   // LIN: %[[AD1:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
-  // LIN: %[[OFA_P1:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD1]], i32 0, i32 2
-  // LIN: %[[OFA1:.+]] = load i8*, i8** %[[OFA_P1]]
-  // LIN: %[[BC1:.+]] = bitcast i8* %[[OFA1]] to i92*
-  // LIN: %[[OFANEXT1:.+]] = getelementptr i8, i8* %[[OFA1]], i32 16
-  // LIN: store i8* %[[OFANEXT1]], i8** %[[OFA_P1]]
+  // LIN: %[[OFA_P1:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD1]], i32 0, i32 0
+  // LIN: %[[GPOFFSET:.+]] = load i32, i32* %[[OFA_P1]]
+  // LIN: %[[FITSINGP:.+]] = icmp ule i32 %[[GPOFFSET]], 32
+  // LIN: br i1 %[[FITSINGP]]
+  // LIN: %[[BC1:.+]] = phi i92*
   // LIN: %[[LOAD1:.+]] = load i92, i92* %[[BC1]]
   // LIN: store i92 %[[LOAD1]], i92*
+
   // WIN: %[[CUR1:.+]] = load i8*, i8** %[[ARGS]]
   // WIN: %[[NEXT1:.+]] = getelementptr inbounds i8, i8* %[[CUR1]], i64 16
   // WIN: store i8* %[[NEXT1]], i8** %[[ARGS]]
@@ -171,15 +172,16 @@
 
   _ExtInt(31) B = __builtin_va_arg(args, _ExtInt(31));
   // LIN: %[[AD2:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
-  // LIN: %[[OFA_P2:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD2]], i32 0, i32 2
-  // LIN: %[[OFA2:.+]] = load i8*, i8** %[[OFA_P2]]
-  // LIN: %[[BC2:.+]] = bitcast i8* %[[OFA2]] to i31*
-  // LIN: %[[OFANEXT2:.+]] = getelementptr i8, i8* %[[OFA2]], i32 8
-  // LIN: store i8* %[[OFANEXT2]], i8** %[[OFA_P2]]
-  // LIN: %[[LOAD2:.+]] = load i31, i31* %[[BC2]]
-  // LIN: store i31 %[[LOAD2]], i31*
+  // LIN: %[[OFA_P2:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD2]], i32 0, i32 0
+  // LIN: %[[GPOFFSET:.+]] = load i32, i32* %[[OFA_P2]]
+  // LIN: %[[FITSINGP:.+]] = icmp ule i32 %[[GPOFFSET]], 40
+  // LIN: br i1 %[[FITSINGP]]
+  // LIN: %[[BC1:.+]] = phi i31*
+  // LIN: %[[LOAD1:.+]] = load i31, i31* %[[BC1]]
+  // LIN: store i31 %[[LOAD1]], i31*
+
   // WIN: %[[CUR2:.+]] = load i8*, i8** %[[ARGS]]
-  // WIN: %[[NEXT2:.+]] = getelementptr inbounds i8, i8* %[[CUR2]], i64 8 
+  // WIN: %[[NEXT2:.+]] = getelementptr inbounds i8, i8* %[[CUR2]], i64 8
   // WIN: store i8* %[[NEXT2]], i8** %[[ARGS]]
   // WIN: %[[BC2:.+]] = bitcast i8* %[[CUR2]] to i31*
   // WIN: %[[LOADV2:.+]] = load i31, i31* %[[BC2]]
@@ -187,13 +189,14 @@
 
   _ExtInt(16) C = __builtin_va_arg(args, _ExtInt(16));
   // LIN: %[[AD3:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
-  // LIN: %[[OFA_P3:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD3]], i32 0, i32 2
-  // LIN: %[[OFA3:.+]] = load i8*, i8** %[[OFA_P3]]
-  // LIN: %[[BC3:.+]] = bitcast i8* %[[OFA3]] to i16*
-  // LIN: %[[OFANEXT3:.+]] = getelementptr i8, i8* %[[OFA3]], i32 8
-  // LIN: store i8* %[[OFANEXT3]], i8** %[[OFA_P3]]
-  // LIN: %[[LOAD3:.+]] = load i16, i16* %[[BC3]]
-  // LIN: store i16 %[[LOAD3]], i16*
+  // LIN: %[[OFA_P3:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD3]], i32 0, i32 0
+  // LIN: %[[GPOFFSET:.+]] = load i32, i32* %[[OFA_P3]]
+  // LIN: %[[FITSINGP:.+]] = icmp ule i32 %[[

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/TargetInfo.cpp:2804
+return;
+  }
+

Quuxplusone wrote:
> Asking silly questions because I don't stand a chance of understanding either 
> the code or the tests...
> 
> In the x86-64 ABI, an ordinary 16-byte `struct` would be passed in `rdi/rsi` 
> and returned in `rdx/rax`. An ordinary 17-byte `struct` would be passed by 
> hidden reference and returned by hidden reference.
> 
> Is your intent here that `_ExtInt` will follow the same convention, i.e., 
> `_ExtInt(128)` will be passed in `rdi/rsi` and returned in `rdx/rax`? and 
> `_ExtInt(129)` will be passed and returned by hidden reference?
> 
> Clang trunk currently passes `_ExtInt(256)` in `rdi/rsi/rdx/rcx/r8/r9/stack` 
> as if it were a series of `uint64_t`s. (I hadn't noticed until now.) That's 
> kind of cute. Is the current PR motivated by performance concerns with the 
> multi-`uint64_t` approach, or just trying to be consistent with the `struct` 
> rules?
Yes, that is my intent.  The bug you filed apparently showed why putting it in 
registers is perhaps a bad idea (for something that large), AND gets us more 
consistent with structs.

As there is no official ABI for any of the 3 platforms, this seems more 
consistent and gives performance benefit.  @craig.topper can explain this part 
better than me.


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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

I think you're missing a check for ExtInt in 
X86_64ABIInfo::getIndirectReturnResult


Repository:
  rC Clang

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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:2804
+return;
+  }
+

Asking silly questions because I don't stand a chance of understanding either 
the code or the tests...

In the x86-64 ABI, an ordinary 16-byte `struct` would be passed in `rdi/rsi` 
and returned in `rdx/rax`. An ordinary 17-byte `struct` would be passed by 
hidden reference and returned by hidden reference.

Is your intent here that `_ExtInt` will follow the same convention, i.e., 
`_ExtInt(128)` will be passed in `rdi/rsi` and returned in `rdx/rax`? and 
`_ExtInt(129)` will be passed and returned by hidden reference?

Clang trunk currently passes `_ExtInt(256)` in `rdi/rsi/rdx/rcx/r8/r9/stack` as 
if it were a series of `uint64_t`s. (I hadn't noticed until now.) That's kind 
of cute. Is the current PR motivated by performance concerns with the 
multi-`uint64_t` approach, or just trying to be consistent with the `struct` 
rules?


Repository:
  rC Clang

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

https://reviews.llvm.org/D78785



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

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

[Erich Keane via Phabricator via cfe-commits]

erichkeane created this revision.
erichkeane added a reviewer: craig.topper.

After speaking with Craig Topper about some recent defects, he pointed
out that _ExtInts should be passed indirectly if larger than the largest
int register, and like ints when smaller than that.  This patch
implements that.

Note that this changed the way vaargs worked quite a bit, but they still
work.


Repository:
  rC Clang

https://reviews.llvm.org/D78785

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/ext-int-sanitizer.cpp
  clang/test/CodeGen/ext-int.c
  clang/test/CodeGenCXX/ext-int.cpp

Index: clang/test/CodeGenCXX/ext-int.cpp
===
--- clang/test/CodeGenCXX/ext-int.cpp
+++ clang/test/CodeGenCXX/ext-int.cpp
@@ -98,7 +98,7 @@
 };
 
 void UnderlyingTypeUsage(AsEnumUnderlyingType Param) {
-  // LIN: define void @_Z19UnderlyingTypeUsage20AsEnumUnderlyingType(i9 %
+  // LIN: define void @_Z19UnderlyingTypeUsage20AsEnumUnderlyingType(i16 %
   // WIN: define dso_local void @"?UnderlyingTypeUsage@@YAXW4AsEnumUnderlyingType@@@Z"(i9 %
   AsEnumUnderlyingType Var;
   // CHECK: alloca i9, align 2
@@ -106,13 +106,13 @@
 }
 
 unsigned _ExtInt(33) ManglingTestRetParam(unsigned _ExtInt(33) Param) {
-// LIN: define i33 @_Z20ManglingTestRetParamU7_ExtIntILi33EEj(i33 %
+// LIN: define i64 @_Z20ManglingTestRetParamU7_ExtIntILi33EEj(i64 %
 // WIN: define dso_local i33 @"?ManglingTestRetParam@@YAU?$_UExtInt@$0CB@@__clang@@U12@@Z"(i33
   return 0;
 }
 
 _ExtInt(33) ManglingTestRetParam(_ExtInt(33) Param) {
-// LIN: define i33 @_Z20ManglingTestRetParamU7_ExtIntILi33EEi(i33 %
+// LIN: define i64 @_Z20ManglingTestRetParamU7_ExtIntILi33EEi(i64 %
 // WIN: define dso_local i33 @"?ManglingTestRetParam@@YAU?$_ExtInt@$0CB@@__clang@@U12@@Z"(i33
   return 0;
 }
@@ -155,13 +155,14 @@
 
   _ExtInt(92) A = __builtin_va_arg(args, _ExtInt(92));
   // LIN: %[[AD1:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
-  // LIN: %[[OFA_P1:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD1]], i32 0, i32 2
-  // LIN: %[[OFA1:.+]] = load i8*, i8** %[[OFA_P1]]
-  // LIN: %[[BC1:.+]] = bitcast i8* %[[OFA1]] to i92*
-  // LIN: %[[OFANEXT1:.+]] = getelementptr i8, i8* %[[OFA1]], i32 16
-  // LIN: store i8* %[[OFANEXT1]], i8** %[[OFA_P1]]
+  // LIN: %[[OFA_P1:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD1]], i32 0, i32 0
+  // LIN: %[[GPOFFSET:.+]] = load i32, i32* %[[OFA_P1]]
+  // LIN: %[[FITSINGP:.+]] = icmp ule i32 %[[GPOFFSET]], 32
+  // LIN: br i1 %[[FITSINGP]]
+  // LIN: %[[BC1:.+]] = phi i92*
   // LIN: %[[LOAD1:.+]] = load i92, i92* %[[BC1]]
   // LIN: store i92 %[[LOAD1]], i92*
+
   // WIN: %[[CUR1:.+]] = load i8*, i8** %[[ARGS]]
   // WIN: %[[NEXT1:.+]] = getelementptr inbounds i8, i8* %[[CUR1]], i64 16
   // WIN: store i8* %[[NEXT1]], i8** %[[ARGS]]
@@ -171,15 +172,16 @@
 
   _ExtInt(31) B = __builtin_va_arg(args, _ExtInt(31));
   // LIN: %[[AD2:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
-  // LIN: %[[OFA_P2:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD2]], i32 0, i32 2
-  // LIN: %[[OFA2:.+]] = load i8*, i8** %[[OFA_P2]]
-  // LIN: %[[BC2:.+]] = bitcast i8* %[[OFA2]] to i31*
-  // LIN: %[[OFANEXT2:.+]] = getelementptr i8, i8* %[[OFA2]], i32 8
-  // LIN: store i8* %[[OFANEXT2]], i8** %[[OFA_P2]]
-  // LIN: %[[LOAD2:.+]] = load i31, i31* %[[BC2]]
-  // LIN: store i31 %[[LOAD2]], i31*
+  // LIN: %[[OFA_P2:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD2]], i32 0, i32 0
+  // LIN: %[[GPOFFSET:.+]] = load i32, i32* %[[OFA_P2]]
+  // LIN: %[[FITSINGP:.+]] = icmp ule i32 %[[GPOFFSET]], 40
+  // LIN: br i1 %[[FITSINGP]]
+  // LIN: %[[BC1:.+]] = phi i31*
+  // LIN: %[[LOAD1:.+]] = load i31, i31* %[[BC1]]
+  // LIN: store i31 %[[LOAD1]], i31*
+
   // WIN: %[[CUR2:.+]] = load i8*, i8** %[[ARGS]]
-  // WIN: %[[NEXT2:.+]] = getelementptr inbounds i8, i8* %[[CUR2]], i64 8 
+  // WIN: %[[NEXT2:.+]] = getelementptr inbounds i8, i8* %[[CUR2]], i64 8
   // WIN: store i8* %[[NEXT2]], i8** %[[ARGS]]
   // WIN: %[[BC2:.+]] = bitcast i8* %[[CUR2]] to i31*
   // WIN: %[[LOADV2:.+]] = load i31, i31* %[[BC2]]
@@ -187,13 +189,14 @@
 
   _ExtInt(16) C = __builtin_va_arg(args, _ExtInt(16));
   // LIN: %[[AD3:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
-  // LIN: %[[OFA_P3:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD3]], i32 0, i32 2
-  // LIN: %[[OFA3:.+]] = load i8*, i8** %[[OFA_P3]]
-  // LIN: %[[BC3:.+]] = bitcast i8* %[[OFA3]] to i16*
-  // LIN: %[[OFANEXT3:.+]] = getelementptr i8, i8* %[[OFA3]], i32 8
-  // LIN: store i8* %[[OFANEXT3]], i8** %[[OFA_P3]]
-  // LIN: %[[LOAD3:.+]] = load i16, i16* %[[BC3]]
-  // LIN: store i16 %[[LOAD3]], i16*
+  // LIN: %[[OFA_P3:.+]] = getelementptr