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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Is it possible to enable fast-isel and/or global-isel for ARM64 Windows?  Do we need additional code for them?  (I'm okay with making those changes separate patches, but it should be easy to at least add testcases now.)



================
Comment at: lib/Target/AArch64/AArch64CallingConvention.td:57
+  CCIfInReg<CCIfType<[i64],
+    CCIfSRet<CCIfType<[i64], CCAssignToRegWithShadow<[X0], [W0]>>>>>,
+
----------------
Instead of the separate CCIfValNo check, can you just write `CCAssignToRegWithShadow<[X0, X1], [W0, W1]>`, like we do for normal i64 arguments?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3435
+    // present.
+    if (TT.isOSWindows() && i->hasStructRetAttr())
+      return false;
----------------
mgrang wrote:
> efriedma wrote:
> > Can we restrict this to only arguments with both sret and inreg?
> > 
> > Maybe leave a FIXME to check whether the callee also has an sret+inreg argument.  That optimization isn't critical, but nice to have at some point.
> You cannot have inreg without sret. So I think just checking inreg should be enough. Will change this to check for inreg. Thanks.
Should we also check for inreg in other places the patch checks for sret?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3218
+        unsigned Reg = FuncInfo->getSRetReturnReg();
+        if (!Reg) {
+          MVT PtrTy = getPointerTy(DAG.getDataLayout());
----------------
We should call LowerFormalArguments exactly once... maybe make this an assertion?


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

https://reviews.llvm.org/D60348