[COFF, ARM64] Decide when to mark struct returns as SRet

[mgrang]

Summary

Refer the MS ARM64 ABI Convention for the behavior for struct returns:
https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions#return-values

[efriedma]

lib/CodeGen/CGCall.cpp

1988 ↗

(On Diff #155998)

We shouldn't be checking the triple here. Can we extend CGFunctionInfo to record this instead, then perform the check in AArch64ABIInfo in lib/CodeGen/TargetInfo.cpp?

1990 ↗

(On Diff #155998)

Are you sure isPOD() is the right check? I wouldn't trust the spec; for other Microsoft targets in other similar situations, we usually check whether the class has a trivial copy constructor, or something like that; see MicrosoftCXXABI::getRecordArgABI.

[yinma]

lib/CodeGen/CGCall.cpp

1988 ↗

(On Diff #155998)

One thing we can do is to add a new ABIKind in AArch64ABIInfo, and test it as the condition

1990 ↗

(On Diff #155998)

Do you have any information about why isPOD is not the right check? The POD definition is defined as the spec. I am seeing clang has many places to setup PlainOldData = false. It doesn't look like it is just about testing constructor for me.

[efriedma]

lib/CodeGen/CGCall.cpp

1990 ↗

(On Diff #155998)

Just verified, you're right, POD is actually the right check. It's just a bit weird because there isn't any technical reason for the calling convention code to check whether the class has, for example, a trivial default constructor.

[mgrang]

Moved the check for NonPODStructRet to CGFunctionInfo.

[rnk]

I think you may want to use a different IR convention to indicate whether X0 or X8 should be used for the pointer to the returned struct. My first thought is to use inreg to indicate that the struct is POD and X8 should be used, and that it does not need to be preserved. In AArch64ABIInfo, you would check for POD-ness and use ABIArgInfo::getIndirectInReg instead of ABIArgInfo::getIndirect.

We should also think about the best way to represent the fact that the address of a non-POD return value must be returned in X0. For X86, the sret attribute is used to communicate this requirement. Another alternative would be to change the LLVM IR function return type to a pointer type and add the LLVM IR returned attribute to the argument with the address of the return value. If we decide not to go with sret + inreg, we need to do this anyway, since the backend has no way of knowing it needs to preserve X0.

lib/CodeGen/CGCall.cpp

1988 ↗

(On Diff #155998)

ABIKind is an implementation detail of AArch64ABIInfo. I'm not sure how we would use that here.

[yinma]

lib/CodeGen/TargetInfo.cpp

4926 ↗

(On Diff #156205)

Do we should limit this to aarch64 windows only? I feel it will impact x86 64bit .

[yinma]

lib/CodeGen/TargetInfo.cpp

4926 ↗

(On Diff #156205)

I just realized Win64 is under AArch64ABIInfo. So this change should be good.

[TomTan]

include/clang/CodeGen/CGFunctionInfo.h

547 ↗

(On Diff #156205)

Use a bit field like ReturnsRetained above?

[efriedma]

My first thought is to use inreg to indicate that the struct is POD and X8 should be used, and that it does not need to be preserved.

If we were going to do this, we'd probably want to reverse the meaning of inreg; I would rather not break compatibility with existing bitcode, particularly on non-Windows targets.

Another alternative would be to change the LLVM IR function return type to a pointer type and add the LLVM IR returned attribute to the argument with the address of the return value.

We do something similar to this for constructors on some targets; see CGCXXABI::HasThisReturn. Probably not hard to extend if we wanted to.

[rnk]

In D49464#1168918, @efriedma wrote:

My first thought is to use inreg to indicate that the struct is POD and X8 should be used, and that it does not need to be preserved.

If we were going to do this, we'd probably want to reverse the meaning of inreg; I would rather not break compatibility with existing bitcode, particularly on non-Windows targets.

I see, I hadn't realized X8 was the normal sret register. Yes, sret + inreg would mean "in the other sret register". However, now I think we should toss this idea and use returned instead, since SysV AArch64 doesn't return the incoming sret pointer in X0, so we'd need to make this new sret+inreg thing imply that.

Another alternative would be to change the LLVM IR function return type to a pointer type and add the LLVM IR returned attribute to the argument with the address of the return value.

We do something similar to this for constructors on some targets; see CGCXXABI::HasThisReturn. Probably not hard to extend if we wanted to.

Yep.

[efriedma]

Reid, are you okay with merging something like the current patch for now, and implementing returning the value as a followup?

lib/CodeGen/CGCall.cpp

1989 ↗

(On Diff #156205)

It would be better to move the isPOD check itself into target/ABI-specific code, more like setSRetAfterThis. But not important either way, I guess.

[rnk]

In D49464#1170686, @efriedma wrote:

Reid, are you okay with merging something like the current patch for now, and implementing returning the value as a followup?

Yeah.

Please add some test cases that verify that the right thing happens for instance methods returning POD structs. Should the sret parameter be in X1 or X8? Looks like you want X1, so isPOD isn't the right check.

struct IsPOD { int x; };
struct Foo {
  IsPOD foo() { return IsPOD{3}; }
};
int main() {
  return Foo().foo().x;
}

include/clang/CodeGen/CGFunctionInfo.h

547 ↗

(On Diff #156205)

Yes, please do not grow this struct needlessly.

Please name it something more generic like SuppressSRet.

lib/CodeGen/TargetInfo.cpp

4926 ↗

(On Diff #156205)

Based on my test case, you should call setSuppressSRet(true) whenever ::classifyReturnType returns true in the MS C++ ABI.

[mgrang]

Addressed comments.

[rnk]

include/clang/CodeGen/CGFunctionInfo.h

521–523 ↗

(On Diff #156928)

This should be a member of the ABIArgInfo, not CGFunctionInfo.

[mgrang]

In D49464#1170710, @rnk wrote:

In D49464#1170686, @efriedma wrote:

Reid, are you okay with merging something like the current patch for now, and implementing returning the value as a followup?

Yeah.

Please add some test cases that verify that the right thing happens for instance methods returning POD structs. Should the sret parameter be in X1 or X8? Looks like you want X1, so isPOD isn't the right check.

struct IsPOD { int x; };
struct Foo {
  IsPOD foo() { return IsPOD{3}; }
};
int main() {
  return Foo().foo().x;
}

The ABI doc just distinguishes between POD and non-POD types.

The address of the memory block shall be passed as an additional argument to the function in x8 for POD type, or in x0 (or x1 if $this is passed in x0) for non-POD type.

So even for instance methods returning PODs, X8 should be used.

[rnk]

In D49464#1172754, @mgrang wrote:

The ABI doc just distinguishes between POD and non-POD types.

The address of the memory block shall be passed as an additional argument to the function in x8 for POD type, or in x0 (or x1 if $this is passed in x0) for non-POD type.

So even for instance methods returning PODs, X8 should be used.

Experimentation shows otherwise: https://godbolt.org/g/jQVW78 X1 is used to pass the sret parameter. Trust but verify. :)

[mgrang]

In D49464#1172767, @rnk wrote:

In D49464#1172754, @mgrang wrote:

The ABI doc just distinguishes between POD and non-POD types.

The address of the memory block shall be passed as an additional argument to the function in x8 for POD type, or in x0 (or x1 if $this is passed in x0) for non-POD type.

So even for instance methods returning PODs, X8 should be used.

Experimentation shows otherwise: https://godbolt.org/g/jQVW78 X1 is used to pass the sret parameter. Trust but verify. :)

Thanks Reid. Yes, you are correct. Instance methods seem to use X1 for struct returns. I have updated my patch accordingly. I guess the doc needs fixing.

[mgrang]

@TomTan @haripul As Reid pointed out, MS ARM64 seems to use X1 for returning POD struct address. See https://godbolt.org/g/jQVW78.
However, the doc at https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions#return-values only mentions the following:

The address of the memory block shall be passed as an additional argument to the function in x8 for POD type, or in x0 (or x1 if $this is passed in x0) for non-POD type.

Could you please check if the doc needs to be fixed?

[efriedma]

Looks fine except for a couple minor nits.

include/clang/CodeGen/CGFunctionInfo.h

99 ↗

(On Diff #157098)

Maybe enforce isIndirect(), like we do for other similar flags?

test/CodeGen/arm64-microsoft-arguments.cpp

1 ↗

(On Diff #157098)

Please use -O0 for CodeGen tests, where possible.

[mgrang]

Addressed Eli's comments.

[efriedma]

LGTM, but please wait to see if Reid has any more comments.

[rnk]

lgtm, thanks!

cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h

private:
   bool PaddingInReg : 1;
   bool InAllocaSRet : 1;    // isInAlloca()
   bool IndirectByVal : 1;   // isIndirect()
   bool IndirectRealign : 1; // isIndirect()
   bool SRetAfterThis : 1;   // isIndirect()
   bool InReg : 1;           // isDirect() || isExtend() || isIndirect()
   bool CanBeFlattened: 1;   // isDirect()
   bool SignExt : 1;         // isExtend()
+  bool SuppressSRet : 1;    // isIndirect()
 
   bool canHavePaddingType() const {
     return isDirect() || isExtend() || isIndirect() || isExpand();
   }
   void setPaddingType(llvm::Type *T) {
     assert(canHavePaddingType());
     PaddingType = T;
   }
 
   void setUnpaddedCoerceToType(llvm::Type *T) {
     assert(isCoerceAndExpand());
     UnpaddedCoerceAndExpandType = T;
   }
 
   ABIArgInfo(Kind K)
-      : TheKind(K), PaddingInReg(false), InReg(false) {
+      : TheKind(K), PaddingInReg(false), InReg(false), SuppressSRet(false) {
   }
 
 public:
   ABIArgInfo()
       : TypeData(nullptr), PaddingType(nullptr), DirectOffset(0),
-        TheKind(Direct), PaddingInReg(false), InReg(false) {}
+        TheKind(Direct), PaddingInReg(false), InReg(false),
+        SuppressSRet(false) {}
 
   static ABIArgInfo getDirect(llvm::Type *T = nullptr, unsigned Offset = 0,
                               llvm::Type *Padding = nullptr,
                               bool CanBeFlattened = true) {
     auto AI = ABIArgInfo(Direct);
     AI.setCoerceToType(T);
     AI.setPaddingType(Padding);
     AI.setDirectOffset(Offset);
bool getCanBeFlattened() const {
     return CanBeFlattened;
   }
 
   void setCanBeFlattened(bool Flatten) {
     assert(isDirect() && "Invalid kind!");
     CanBeFlattened = Flatten;
   }
 
+  bool getSuppressSRet() const {
+    assert(isIndirect() && "Invalid kind!");
+    return SuppressSRet;
+  }
+
+  void setSuppressSRet(bool Suppress) {
+    assert(isIndirect() && "Invalid kind!");
+    SuppressSRet = Suppress;
+  }
+
   void dump() const;
 };
 
 /// A class for recording the number of arguments that a function
 /// signature requires.
 class RequiredArgs {
   /// The number of required arguments, or ~0 if the signature does
   /// not permit optional arguments.

cfe/trunk/lib/CodeGen/CGCall.cpp

void CodeGenModule::ConstructAttributeList(
   }
 
   bool hasUsedSRet = false;
   SmallVector<llvm::AttributeSet, 4> ArgAttrs(IRFunctionArgs.totalIRArgs());
 
   // Attach attributes to sret.
   if (IRFunctionArgs.hasSRetArg()) {
     llvm::AttrBuilder SRETAttrs;
+    if (!RetAI.getSuppressSRet())
       SRETAttrs.addAttribute(llvm::Attribute::StructRet);
     hasUsedSRet = true;
     if (RetAI.getInReg())
       SRETAttrs.addAttribute(llvm::Attribute::InReg);
     ArgAttrs[IRFunctionArgs.getSRetArgNo()] =
         llvm::AttributeSet::get(getLLVMContext(), SRETAttrs);
   }
 
   // Attach attributes to inalloca argument.

cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp

if (!RD)
     return false;
 
   CharUnits Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
   if (FI.isInstanceMethod()) {
     // If it's an instance method, aggregates are always returned indirectly via
     // the second parameter.
     FI.getReturnInfo() = ABIArgInfo::getIndirect(Align, /*ByVal=*/false);
     FI.getReturnInfo().setSRetAfterThis(FI.isInstanceMethod());
+
+    // aarch64-windows requires that instance methods use X1 for the return
+    // address. So for aarch64-windows we do not mark the
+    // return as SRet.
+    FI.getReturnInfo().setSuppressSRet(CGM.getTarget().getTriple().getArch() ==
+                                       llvm::Triple::aarch64);
     return true;
   } else if (!RD->isPOD()) {
     // If it's a free function, non-POD types are returned indirectly.
     FI.getReturnInfo() = ABIArgInfo::getIndirect(Align, /*ByVal=*/false);
+
+    // aarch64-windows requires that non-POD, non-instance returns use X0 for
+    // the return address. So for aarch64-windows we do not mark the return as
+    // SRet.
+    FI.getReturnInfo().setSuppressSRet(CGM.getTarget().getTriple().getArch() ==
+                                       llvm::Triple::aarch64);
     return true;
   }
 
   // Otherwise, use the C ABI rules.
   return false;
 }
 
 llvm::BasicBlock *

cfe/trunk/test/CodeGen/arm64-microsoft-arguments.cpp

+// RUN: %clang_cc1 -triple aarch64-windows -ffreestanding -emit-llvm -O0 \
+// RUN: -x c++ -o - %s | FileCheck %s
+
+struct pod { int a, b, c, d, e; };
+
+struct non_pod {
+  int a;
+  non_pod() {}
+};
+
+struct pod s;
+struct non_pod t;
+
+struct pod bar() { return s; }
+struct non_pod foo() { return t; }
+// CHECK: define {{.*}} void @{{.*}}bar{{.*}}(%struct.pod* noalias sret %agg.result)
+// CHECK: define {{.*}} void @{{.*}}foo{{.*}}(%struct.non_pod* noalias %agg.result)
+
+
+// Check instance methods.
+struct pod2 { int x; };
+struct Baz { pod2 baz(); };
+
+int qux() { return Baz().baz().x; }
+// CHECK: declare {{.*}} void @{{.*}}baz@Baz{{.*}}(%struct.Baz*, %struct.pod2*)