-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ms_abi is implemented incorrectly for larger values (>=16 bytes) #30710
Comments
*** Bug llvm/llvm-bugzilla-archive#33834 has been marked as a duplicate of this bug. *** |
*** Bug #29513 has been marked as a duplicate of this bug. *** |
*** Bug llvm/llvm-bugzilla-archive#35814 has been marked as a duplicate of this bug. *** |
Issue 35814 was tentatively marked as a release blocker, so let's mark this as such too. I don't know if anyone will have time to look into it though. |
Bug 35814 was specifically about clang now asserting after https://reviews.llvm.org/rL316416. It would be nice to fix at least that, somehow? Not sure if reverting r316416 is feasible, though. |
I plan to look into this issue. Currently my understanding is that win64cc was never fully implemented (byval returns and arguments are not handled as specified in the convention). The solution seems to be to modify X86ISelLowering to correctly spill/fill byval arguments to addresses passed in RCX, RDX, R8 and R9 instead of stack if this CC is used. With byvals loaded and stored correctly the original problem form Bug 35814 should disappear as the X86CallFrameOptimization will not be removing the stores and loads anymore. |
After investigation I reached conclusion that issue is caused by Clang. When clang is lowering function into IR on platform other than Windows standard x86_64 ABI rules are used with disregard to what is specified using attribute. This is caused by the fact that only compilation target is checked when when choosing ABI, calling convention of lowered function (which may be overwritten by attribute) is not checked. This caused clang to split 128-bit byval into 64-bit arguments when emiting IR. Even though the information about CC is available to the LLVM code generator there is no indication that the arguments were originally forming single byval argument, so code gen is not able to produce correct binary in this case. I've prepared a patch that is fixing this issue: https://reviews.llvm.org/D43016 |
There's now a fix landed in https://reviews.llvm.org/rL324594, it at least fixes the assertion from bug 35814, but can the original submitter of this bug also please independently verify that it fixes their issue? We should merge this as soon as it has baked a little in trunk, Hans, do you agree? |
Sounds good to me. Merged in r324718. |
mentioned in issue llvm/llvm-bugzilla-archive#33834 |
mentioned in issue #35152 |
mentioned in issue llvm/llvm-bugzilla-archive#35814 |
mentioned in issue llvm/llvm-bugzilla-archive#36806 |
Extended Description
This code compiles to assembly that looks like this:
As per these two documents:
Both of these are wrong and the generated assembly ought to look like this instead:
The relevant excerpts:
Also from https://msdn.microsoft.com/en-us/library/ms235286.aspx:
The text was updated successfully, but these errors were encountered: