Refer the ABI doc at: https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#return-values
Related clang patch: D60349
Differential D60348
[COFF, ARM64] Fix ABI implementation of struct returns mgrang on Apr 5 2019, 5:32 PM. Authored by
Details
Refer the ABI doc at: https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#return-values Related clang patch: D60349
Diff Detail
Event TimelineComment Actions Refer https://bugs.llvm.org/show_bug.cgi?id=41135 for a detailed discussion on the issue. Please review this and the related clang patch and let me know if this implementation is something we can proceed with. Note: In case, for instance methods (when "this" is passed in x0), we would need to do a mov from x1 to get the sret parameter. I have not yet implemented this. Will do so once the reviewers +1 the implementation. Comment Actions Hi Mandeep, I had a quick look through and wrote up some thoughts that triggered immediately. Thanks!
Comment Actions Hi Mandeep, I spent a bit more time understanding the code in more depth now. I expect to be offline for the next 7 days, so if anyone else would want to review/approve an updated version of this patch: I'd also be happy with that if the above comments have been taken into account.
Comment Actions Thanks @kristof.beyls for the review. We are still working on the combining the clang patches which should fix PR41135 and PR41136. I will then add unit tests to this patch. Comment Actions Fair enough, I just got back to work, so I'll look into it. :) From what I can tell, the last update implements all the suggested simplifications. I have a suggestion, but maybe it cuts against the direction you were suggesting. Overall, I'm happy with this.
Comment Actions Do you need to modify AArch64TargetLowering::isEligibleForTailCallOptimization to prevent a tail call in cases where the tail call would return the wrong value? Comment Actions Thanks Eli. I have prevented tail call for arm64-windows when sret arguments are present.
Comment Actions 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 Actions GlobalISEL is the default for unoptimized builds, and iirc FastISEL should work as well (it was pretty well working already when GlobalISEL was enabled by default for aarch64).
Comment Actions I went over other reviewers comments, and I think this was the only open one: Based on the comments in the test, I take it this will be addressed in a subsequent patch. Sounds good. lgtm
|