[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/AArch64CallingConvention.td:39-51
+  // In AAPCS, an SRet is passed in X8, not X0 like a normal pointer parameter.
+  // However, on windows, in some circumstances, the SRet is passed in X0 or X1
+  // instead.  The presence of the inreg attribute indicates that SRet is
+  // passed in the alternative register (X0 or X1), not X8:
+  // - X0 for non-instance methods.
+  // - X1 for instance methods.
+
----------------
rnk wrote:
> I feel like the whole discussion of free function vs. C++ instance method (X0/X1) could be avoided by falling through to the normal i64 argument handling below. IIUC, these are roughly the conditions we want:
>   if (IsSRet && (IsInReg || !IsWin64))
>     // assign to X8
> I'm not sure how to express that in our calling conv tablegen, though.
Nice, I think this is simpler.


================
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
----------------
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.


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

https://reviews.llvm.org/D60348