[PATCH] D60348: [COFF, ARM64] Fix ABI implementation of struct returns

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3430
 
+    // On Windows, "inreg" attributes signify non-aggregate types which must be
+    // returned indirectly. In this case, it is necessary to save/restore X0 in
----------------
mgrang wrote:
> efriedma wrote:
> > rnk wrote:
> > > mgrang wrote:
> > > > rnk wrote:
> > > > > Hang on, why are we checking for `inreg` at all? Won't we still erroneously perform TCO on this example that we currently TCO?
> > > > > ```
> > > > > struct Foo {
> > > > >   Foo();
> > > > >   Foo(const Foo &o);
> > > > >   Foo(int, int);
> > > > >   ~Foo();
> > > > >   void *p;
> > > > > };
> > > > > 
> > > > > void bar(Foo *, int, int);
> > > > > 
> > > > > Foo getfoo(int x, int y) {
> > > > >   Foo f;
> > > > >   bar(&f, x, y);
> > > > >   return f;
> > > > > }
> > > > > ```
> > > > > In this example, NRVO fires, and `&f` is the sret pointer passed in. `bar` isn't guaranteed to return `&f`, but we tail call it, and `inreg` isn't involved at all.
> > > > > 
> > > > > I think the precise check that we are looking for is that, if the caller has an arg marked sret, then that argument must be passed to the callee and be marked `sret`. It doesn't matter if the callee receives the sret pointer in a different argument (X0, X1, X8), what matters is that it's returned in X0, and that matches the caller function's requirements. It might also be worth enabling TCO on constructors, which use the `returned` convention in IR, so something like this:
> > > > > ```
> > > > > Foo getfoo(int x, int y) {
> > > > >   return Foo(x, y);
> > > > > }
> > > > > ```
> > > > > 
> > > > > Bailing on the caller using sret is conservatively correct of course.
> > > > Thanks Reid. I see in other targets like RISCV we disable TCO if caller or callee use StructRet. Should we do the same here? Also should we restrict that to only Win64?
> > > I suppose yes, restricting it to Win64 makes sense, since that's where the preservation requirement comes from.
> > > Hang on, why are we checking for inreg at all?
> > 
> > "inreg" specifically marks the calls where we have to restore x0.  For normal C calls, we follow the usual AArch64 ABI rules, and don't need to preserve the sret address.
> I agree with Eli. We need to avoid TCO when we have to restore X0. The "sret" attribute is also used for aggregates larger than 16 bytes where the address is passed in X8 but no register needs to be restored.
> 
> Like here:
> ```
> struct S3 { int a, b, c, d, e; };
> S3 f3() { return S3{}; }
> 
> define dso_local void @"?f3@@YA?AUS3@@XZ"(%struct.S3* noalias nocapture sret %agg.result)
> ```
> 
> The "inreg" attribute specifies that X0 needs to be restored. So I guess we should indeed check for "inreg" (not "sret").
Sorry, my misunderstanding. I thought the sret pointer was returned in all cases: aggregate, non-aggregate, instance method or otherwise, but I see it's only for non-aggregates.


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

https://reviews.llvm.org/D60348