[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

1989

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?

1991

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

1989

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

1991

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

1991

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

1989

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

[yinma]

lib/CodeGen/TargetInfo.cpp

4926

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

[yinma]

lib/CodeGen/TargetInfo.cpp

4926

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

[TomTan]

include/clang/CodeGen/CGFunctionInfo.h

550

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

1990

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

550

Yes, please do not grow this struct needlessly.

Please name it something more generic like SuppressSRet.

lib/CodeGen/TargetInfo.cpp

4926

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

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

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

test/CodeGen/arm64-microsoft-arguments.cpp

2

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!

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 canHavePaddingType() const {
     return isDirect() || isExtend() || isIndirect() || isExpand();
   }
   void setPaddingType(llvm::Type *T) {
     assert(canHavePaddingType());
     PaddingType = T;
   }
 
class CGFunctionInfo final
 
   /// How many arguments to pass inreg.
   unsigned HasRegParm : 1;
   unsigned RegParm : 3;
 
   /// Whether this function has nocf_check attribute.
   unsigned NoCfCheck : 1;
 
+  /// Whether the SRet attribute should be suppressed for this function.
+  unsigned SuppressSRet : 1;
+
   RequiredArgs Required;
 
   /// The struct representing all arguments passed in memory.  Only used when
   /// passing non-trivial types with inalloca.  Not part of the profile.
   llvm::StructType *ArgStruct;
   unsigned ArgStructAlign : 31;
   unsigned HasExtParameterInfos : 1;
 
ExtParameterInfo *getExtParameterInfosBuffer() {
     return getTrailingObjects<ExtParameterInfo>();
   }
   const ExtParameterInfo *getExtParameterInfosBuffer() const{
     return getTrailingObjects<ExtParameterInfo>();
   }
 
   CGFunctionInfo() : Required(RequiredArgs::All) {}
 
 public:
   static CGFunctionInfo *create(unsigned llvmCC,
                                 bool instanceMethod,
                                 bool chainCall,
                                 const FunctionType::ExtInfo &extInfo,
                                 ArrayRef<ExtParameterInfo> paramInfos,
                                 CanQualType resultType,
                                 ArrayRef<CanQualType> argTypes,
                                 RequiredArgs required);
public:
   bool isReturnsRetained() const { return ReturnsRetained; }
 
   /// Whether this function no longer saves caller registers.
   bool isNoCallerSavedRegs() const { return NoCallerSavedRegs; }
 
   /// Whether this function has nocf_check attribute.
   bool isNoCfCheck() const { return NoCfCheck; }
 
+  /// Whether the SRet attribute should be suppresed for this function.
+  bool isSuppressSRet() const { return SuppressSRet; }
+  void setSuppressSRet(bool Value) { SuppressSRet = Value; }
+
   /// getASTCallingConvention() - Return the AST-specified calling
   /// convention.
   CallingConv getASTCallingConvention() const {
     return CallingConv(ASTCallingConvention);
   }
 
   /// getCallingConvention - Return the user specified calling
   /// convention, which has been translated into an LLVM CC.

lib/CodeGen/CGCall.cpp

CGFunctionInfo *CGFunctionInfo::create(unsigned llvmCC,
   FI->Required = required;
   FI->HasRegParm = info.getHasRegParm();
   FI->RegParm = info.getRegParm();
   FI->ArgStruct = nullptr;
   FI->ArgStructAlign = 0;
   FI->NumArgs = argTypes.size();
   FI->HasExtParameterInfos = !paramInfos.empty();
   FI->getArgsBuffer()[0].type = resultType;
+  FI->SuppressSRet = 0;
   for (unsigned i = 0, e = argTypes.size(); i != e; ++i)
     FI->getArgsBuffer()[i + 1].type = argTypes[i];
   for (unsigned i = 0, e = paramInfos.size(); i != e; ++i)
     FI->getExtParameterInfosBuffer()[i] = paramInfos[i];
   return FI;
 }
 
 /***/
void CodeGenModule::ConstructAttributeList(
   }
 
   bool hasUsedSRet = false;
   SmallVector<llvm::AttributeSet, 4> ArgAttrs(IRFunctionArgs.totalIRArgs());
 
   // Attach attributes to sret.
   if (IRFunctionArgs.hasSRetArg()) {
     llvm::AttrBuilder SRETAttrs;
+    if (FI.isSuppressSRet()) {
+      const CXXRecordDecl *RD = FI.getReturnType()->getAsCXXRecordDecl();
+      if (RD && RD->isPOD())
+        SRETAttrs.addAttribute(llvm::Attribute::StructRet);
+    } else
       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.

lib/CodeGen/TargetInfo.cpp

bool isHomogeneousAggregateSmallEnough(const Type *Ty,
                                          uint64_t Members) const override;
 
   bool isIllegalVectorType(QualType Ty) const;
 
   void computeInfo(CGFunctionInfo &FI) const override {
     if (!::classifyReturnType(getCXXABI(), FI, *this))
       FI.getReturnInfo() = classifyReturnType(FI.getReturnType());
 
+    FI.setSuppressSRet(Kind == Win64);
+
     for (auto &it : FI.arguments())
       it.info = classifyArgumentType(it.type);
   }
 
   Address EmitDarwinVAArg(Address VAListAddr, QualType Ty,
                           CodeGenFunction &CGF) const;
 
   Address EmitAAPCSVAArg(Address VAListAddr, QualType Ty,

test/CodeGen/arm64-microsoft-arguments.cpp

+// RUN: %clang_cc1 -triple aarch64-windows -ffreestanding -emit-llvm \
+// 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() { return pod2 {3}; }
+};
+
+int qux() { return Baz().baz().x; }
+// CHECK: define {{.*}} void @{{.*}}baz@Baz{{.*}}(%struct.Baz* %this, %struct.pod2* noalias sret %agg.result)