Page MenuHomePhabricator

[MS] Overhaul how clang passes overaligned args on x86_32
ClosedPublic

Authored by rnk on Jan 2 2020, 3:25 PM.

Details

Summary

MSVC 2013 would refuse to pass highly aligned things (typically vectors
and aggregates) by value. Users would receive this error:

t.cpp(11) : error C2719: 'w': formal parameter with __declspec(align('32')) won't be aligned
t.cpp(11) : error C2719: 'q': formal parameter with __declspec(align('32')) won't be aligned

However, in MSVC 2015, this behavior was changed, and highly aligned
things are now passed indirectly. To avoid breaking backwards
incompatibility, objects that do not have a *required* high alignment
(i.e. double) are still passed directly, even though they are not
naturally aligned. This change implements the new behavior of passing
things indirectly.

The new behavior is:

  • up to three vector parameters can be passed in [XYZ]MM0-2
  • remaining arguments with required alignment greater than 4 bytes are passed indirectly

Previously, MSVC never passed things truly indirectly, meaning clang
would always apply the byval attribute to indirect arguments. We had to
go to the trouble of adding inalloca so that non-trivially copyable C++
types could be passed in place without copying the object
representation. When inalloca was added, we asserted that all arguments
passed indirectly must use byval. With this change, that assert no
longer holds, and I had to update inalloca to handle that case. The
implicit sret pointer parameter was already handled this way, and this
change generalizes some of that logic to arguments.

There are two cases that this change leaves unfixed:

  1. objects that are non-trivially copyable *and* overaligned
  2. vectorcall + inalloca + vectors

For case 1, I need to touch C++ ABI code in MicrosoftCXXABI.cpp, so I
want to do it in a follow-up.

For case 2, my fix is one line, but it will require updating IR tests to
use lots of inreg, so I wanted to separate it out.

Related to D71915 and D72110

Fixes most of PR44395

Diff Detail

Event Timeline

rnk created this revision.Jan 2 2020, 3:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 2 2020, 3:25 PM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

I think this is alright, I want @ctopper to take a look before I approve it though. Additionally, do you know if this modifies the regcall calling convention at all? Should it?

clang/test/CodeGenCXX/inalloca-vector.cpp
72

Are all the checks hre on out disabled for a reason?

craig.topper added inline comments.Jan 3 2020, 10:55 AM
clang/test/CodeGen/x86_32-arguments-win32.c
77

What happens in the backend with inreg if 512-bit vectors aren't legal?

rjmccall added inline comments.Jan 3 2020, 11:39 AM
clang/include/clang/CodeGen/CGFunctionInfo.h
91

Would it be better to handle inalloca differently, maybe as a flag rather than as a top-level kind? I'm concerned about gradually duplicating a significant amount of the expressivity of other kinds.

rnk marked 2 inline comments as done.Jan 3 2020, 11:54 AM
rnk added inline comments.
clang/test/CodeGen/x86_32-arguments-win32.c
77

LLVM splits the vector up using the largest legal vector size. As many pieces as possible are passed in available XMM/YMM registers, and the rest are passed in memory. MSVC, of course, assumes the user wanted the larger vector size, and uses whatever vector instructions it needs to move the arguments around.

Previously, I advocated for a model where calling an Intel intrinsic function had the effect of implicitly marking the caller with the target attributes of the intrinsic. This falls down if the user tries to write a single function that conditionally branches between code that uses different instruction set extensions. You can imagine the SSE2 codepath accidentally using AVX instructions because the compiler thinks they are better. I'm told that ICC models CPU micro-architectural features in the CFG, but I don't ever expect that LLVM will do that. If we're stuck with per-function CPU feature settings, it seems like it would be nice to try to do what the user asked by default, and warn the user if we see them doing a cpuid check in a function that has been implicitly blessed with some target attributes. You could imagine doing a similar thing when large vector type variables are used: if a large vector argument or local is used, implicitly enable the appropriate target features to move vectors of that size around.

This idea didn't get anywhere, and the current situation has persisted.


You know, maybe we should just keep clang the way it is, and just set up a warning in the backend that says "hey, I split your large vector. You probably didn't want that." And then we just continue doing what we do now. Nobody likes backend warnings, but it seems better than the current direction of the frontend knowing every detail of x86 vector extensions.

clang/test/CodeGenCXX/inalloca-vector.cpp
72

Yes, this is case 2 in the commit message. I won't close the bug without coming back to this.

rjmccall added inline comments.Jan 3 2020, 12:21 PM
clang/test/CodeGen/x86_32-arguments-win32.c
77

If target attributes affect ABI, it seems really dangerous to implicitly set attributes based on what intrinsics are called.

The local CPU-testing problem seems similar to the problems with local #pragma STDC FENV_ACCESS blocks that the constrained-FP people are looking into. They both have a "this operation is normally fully optimizable, but we might need to be more careful in specific functions" aspect to them. I wonder if there's a reasonable way to unify the approaches, or at least benefit from lessons learned.

rnk marked 2 inline comments as done.Jan 3 2020, 2:44 PM
rnk added inline comments.
clang/include/clang/CodeGen/CGFunctionInfo.h
91

In the past, I've drafted a more than one unfinished designs for how we could remodel inalloca with tokens so that it can be per-argument instead of something that applies to all argument memory. Unfortunately, I never found the time to finish or implement one.

As I was working on this patch, I was thinking to myself that this could be the moment to implement one of those designs, but it would be pretty involved. Part of the issue is that, personally, I have very little interest in improving x86_32 code quality, so a big redesign wouldn't deliver much benefit. The benefits would all be code simplifications and maintenance cost reductions, which are nice, but seem to only get me through the prototype design stage.

I'll go dig up my last doc and try to share it, but for now, I think we have to suffer the extra inalloca code in this patch.

clang/test/CodeGen/x86_32-arguments-win32.c
77

I agree, we wouldn't want intrinsic usage to change ABI. But, does anybody actually want the behavior that LLVM implements today where large vectors get split across registers and memory? I think most users who pass a 512 bit vector want it to either be passed in ZMM registers or fail to compile. Why do we even support passing 1024 bit vectors? Could we make that an error?

Anyway, major redesigns aside, should clang do something when large vectors are passed? Maybe we should warn here? Passing by address is usually the safest way to pass something, so that's an option. Implementing splitting logic in clang doesn't seem worth it.

rnk marked an inline comment as done.Jan 3 2020, 3:02 PM
rnk added inline comments.
clang/include/clang/CodeGen/CGFunctionInfo.h
91

Here's what I wrote, with some sketches of possible LLVM IR that could replace inalloca:
https://reviews.llvm.org/P8183

The basic idea is that we need a call setup instruction that forms a region with the call. During CodeGen, we can look forward (potentially across BBs) to determine how much argument stack memory to allocate, allocate it (perhaps in pieces as we go along), and then skip the normal call stack argument adjustment during regular call lowering.

Suggestions for names better than "argmem" are welcome.

The major complication comes from edges that exit the call setup region. These could be exceptional exits or normal exits with statement expressions and break, return, or goto. Along these paths we need to adjust SP, or risk leaking stack memory. Today, with inalloca, I think we leak stack memory.

rjmccall added inline comments.Jan 3 2020, 5:47 PM
clang/include/clang/CodeGen/CGFunctionInfo.h
91

In the past, I've drafted a more than one unfinished designs for how we could remodel inalloca with tokens so that it can be per-argument instead of something that applies to all argument memory. Unfortunately, I never found the time to finish or implement one.

Sorry! I think it would be great to rethink inalloca to avoid the duplication and so on, but I certainly didn't mean to suggest that we should do that as part of this patch. (I'll look at your proposal later.) I was trying to ask if it would make sense to change how inalloca arguments are represented by ABIInfo, so that we could e.g. build a normal indirect ABIInfo and then flag that it also needs to be written into an inalloca buffer.

clang/test/CodeGen/x86_32-arguments-win32.c
77

I agree, we wouldn't want intrinsic usage to change ABI. But, does anybody actually want the behavior that LLVM implements today where large vectors get split across registers and memory?

I take it you're implying that the actual (Windows-only?) platform ABI doesn't say anything about this because other compilers don't allow large vectors. How large are the vectors we do have ABI rules for? Do they have the problem as the SysV ABI where the ABI rules are sensitive to compiler flags?

Anyway, I didn't realize the i386 Windows ABI *ever* used registers for arguments. (Whether you can convince LLVM to do so for a function signature that Clang isn't supposed to emit for ABI-conforming functions is a separate story.) You're saying it uses them for vectors? Presumably up to some limit, and only when they're non-variadic arguments?

AntonYudintsev added inline comments.
clang/test/CodeGen/x86_32-arguments-win32.c
77

You're saying it uses them for vectors? Presumably up to some limit, and only when they're non-variadic arguments?

Sorry to cut in (I am the one who report the issue, and so looking forward for this patch to be merged).
Yes, MSVC/x86 win ABI uses three registers for first three arguments.

https://godbolt.org/z/PZ3dBa

rjmccall added inline comments.Jan 7 2020, 12:01 AM
clang/test/CodeGen/x86_32-arguments-win32.c
77

Interesting. And anything that would caused the stack to be used is still banned: passing more than 3 vectors or passing them as variadic arguments.

I guess they chose not to implement stack realignment when they implemented this, and rather than being stuck with a suboptimal ABI, they just banned the cases that would have required it. Technically that means that they haven't committed to an ABI, so even though LLVM is perfectly happy to realign the stack when required, we shouldn't actually take advantage of that here, and instead we should honor the same restriction.

AntonYudintsev added inline comments.Jan 7 2020, 7:51 AM
clang/test/CodeGen/x86_32-arguments-win32.c
77

As I mentioned in https://reviews.llvm.org/D71915 ( and in https://bugs.llvm.org/show_bug.cgi?id=44395 )

there is at least one particular case, where they do align stack for aligned arguments, although it is not directly a fun call.

rjmccall added inline comments.Jan 7 2020, 9:29 AM
clang/test/CodeGen/x86_32-arguments-win32.c
77

Oh, I see. Then they *have* in fact implemented stack realignment, sometime between MSVC v19.10 and v19.14, so the ABI is now settled; I should've checked a more recent compiler than the one you linked. So this is now a fairly standard in-registers-up-to-a-limit ABI, except that the limit is only non-zero for vectors. (Highly-aligned struct types are passed indirectly.)

AntonYudintsev added inline comments.Jan 8 2020, 12:20 AM
clang/test/CodeGen/x86_32-arguments-win32.c
77

Sorry for not being clear enough :(.

Yes, newer MSVC seem to have implemented stack realignment universally.

Older one (19.10) only did that in a particular cases (like in the one I mentioned in my original patchset).

rnk updated this revision to Diff 238152.Jan 14 2020, 5:58 PM
rnk marked 2 inline comments as done.
  • rebase
clang/include/clang/CodeGen/CGFunctionInfo.h
91

I see. I think implementing that would require a greater refactoring of ClangToLLVMArgMapping. We'd need a new place to store the inalloca struct field index. That is currently put in a union with some other stuff, which would no longer work.

clang/test/CodeGen/x86_32-arguments-win32.c
77

To clarify, it's not actually stack realignment, they pass highly aligned things indirectly to avoid having to implement stack realignment. (https://godbolt.org/z/XwKuyC) Realigning the stack for arguments would be hard for MSVC, because the 32-bit compiler always pushes arguments in memory individually, adjusting the stack as it goes along. My patch implements that logic in the frontend, which is necessary for over aligned aggregates anyway, if not vectors.

Unit tests: fail. 61871 tests passed, 1 failed and 781 were skipped.

failed: Clang.PCH/ms-pch-macro.c

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

rjmccall added inline comments.Jan 16 2020, 10:15 AM
clang/include/clang/CodeGen/CGFunctionInfo.h
91

Okay. Then in the short term, I guess this is fine, and the long-term improvement is to change the inalloca design.

This revision is now accepted and ready to land.Wed, Jan 22, 8:29 AM
This revision was automatically updated to reflect the committed changes.