This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mgrang on Jul 17 2018, 6:12 PM.

Diff Detail

Event Timeline

mgrang created this revision.Jul 17 2018, 6:12 PM
efriedma added inline comments.
lib/CodeGen/CGCall.cpp
1988

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

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 added a subscriber: yinma.Jul 18 2018, 12:10 PM
yinma added inline comments.
lib/CodeGen/CGCall.cpp
1988

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

1990

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 added inline comments.Jul 18 2018, 12:38 PM
lib/CodeGen/CGCall.cpp
1990

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 updated this revision to Diff 156205.Jul 18 2018, 7:32 PM

Moved the check for NonPODStructRet to CGFunctionInfo.

rnk added a comment.Jul 19 2018, 10:27 AM

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

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

yinma added inline comments.Jul 19 2018, 10:50 AM
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 added inline comments.Jul 19 2018, 11:22 AM
lib/CodeGen/TargetInfo.cpp
4926 ↗(On Diff #156205)

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

TomTan added a subscriber: TomTan.Jul 19 2018, 2:50 PM
TomTan added inline comments.
include/clang/CodeGen/CGFunctionInfo.h
557

Use a bit field like ReturnsRetained above?

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 added a comment.Jul 19 2018, 4:05 PM

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.

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

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 added a comment.Jul 20 2018, 4:51 PM

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
557

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 updated this revision to Diff 156928.Jul 23 2018, 4:56 PM

Addressed comments.

rnk added inline comments.Jul 23 2018, 4:59 PM
include/clang/CodeGen/CGFunctionInfo.h
531–533

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

In D49464#1170710, @rnk 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 added a comment.Jul 23 2018, 5:08 PM

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 updated this revision to Diff 157098.Jul 24 2018, 12:06 PM
mgrang retitled this revision from [COFF, ARM64] Mark only POD-type returns as SRET to [COFF, ARM64] Decide when to mark struct returns as SRet.
mgrang edited the summary of this revision. (Show Details)
In D49464#1172767, @rnk 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.

@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?

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
1

Please use -O0 for CodeGen tests, where possible.

mgrang updated this revision to Diff 157164.Jul 24 2018, 4:22 PM

Addressed Eli's comments.

mgrang marked 5 inline comments as done.Jul 24 2018, 4:22 PM
efriedma accepted this revision.Jul 24 2018, 5:11 PM

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

This revision is now accepted and ready to land.Jul 24 2018, 5:11 PM
rnk accepted this revision.Jul 26 2018, 10:36 AM

lgtm, thanks!

This revision was automatically updated to reflect the committed changes.