[stdlib][SR-2239]: add support for AArch64 variadics #20862
Conversation
|
@swift-ci test |
|
@tkremenek This is awesome! Thanks. |
|
@futurejones Glad to see this make progress, but I merged this too quickly. For some reason I thought the patch had been reviewed. @rjmccall @compnerd can one of you please review this? We can either revert this change pending review, or follow up with another change based on feedback. |
| if (ClangCtx.getTargetInfo().getBuiltinVaListKind() != | ||
| clang::TargetInfo::AArch64ABIBuiltinVaList) | ||
| return std::make_pair(Type(), ""); | ||
| } |
There was a problem hiding this comment.
I think that the result of the discussion was that we wanted to just simplify this to:
switch (ClangCtx.getTargetInfo().getBuiltinVaListKind()) {
case clang::TargetInfo::BuiltinVaListKind::CharPtrBuiltinVaList:
case clang::TargetInfo::BuiltinVaListKind::VoidPtrBuiltinVaList:
case clang::TargetInfo::BuiltinVaListKind::PowerABIBuiltinVaList:
case clang::TargetInfo::BuiltinVaListKind::X86_64ABIBuiltinVaList:
case clang::TargetInfo::BuiltinVaListKind::AAPCSABIBuiltinVaList:
assert(ClangCtx.getTypeSize(ClangCtx.VoidPtrTy) == ClangTypeSize &&
"expected va_list type to be sizeof(void *)");
break;
case clang::TargetInfo::BuiltinVaListKind::AArch64ABIBuiltinVaList:
break;
default: return std::make_pair(Type(), "");
}
There was a problem hiding this comment.
Yes, we could simplify a lot of things throughout the entire Swift code base, but the purpose of this PR is to add support for the aarch64/Linux platform. Not to edit code affecting the support of any other platforms.
There was a problem hiding this comment.
This is not a good attitude. As a contributor, you are responsible for your contribution and its impact on the overall codebase. We're not asking you to make random unrelated changes, we're asking to make changes that specifically make your contribution — and changes like it — easier to write and maintain.
To put it another way, if you're going to ask every other Swift contributor to make an effort to keep your port functional as they work on the project over the years, we can ask you to put more than the minimum possible effort into your initial patches.
There was a problem hiding this comment.
@rjmccall I think you have misinterpreted my reply. We are trying to introduce support for a new platform AArch64/Linux. We want to do this with little or no impact on existing supported platforms. These are the first steps towards achieving successful builds on AArch64/Linux. As AArch64/Linux is not yet an "officially" supported platform the last thing we want to do at this point is to start refactoring code that can potentially effect existing supported platforms.
FYI. @rjmccall @jrose-apple I have invested a huge amount of time and considerable money into the Swift on AArch64/Linux project over the past 2 years. Is it really necessary to suggest I am only putting in
the minimum possible effort
There was a problem hiding this comment.
I didn't mean to disparage the effort you've put into this port, and I apologize for that.
It has been my experience dealing with this kind of thing — across multiple compilers — that new ports almost inevitably push the codebase in at least one novel way. Not only is it acceptable to take these opportunities to refactor code that affects existing supported platforms, but it's actively desirable. The alternative is that each new port re-learns how to solve the same set of problems and just contributes to an ever-mounting pile of target-specific checks which none of the other maintainers will really understand the purpose of, when in reality there's nearly always some underlying logic that can be applied both across the codebase and across targets.
Minimizing the impact of a patch is appreciated and is usually the right starting point, but it's not a project goal in and of itself to achieve that on a patch-by-patch basis — the project goal is to accomplish something while still ending up with the best code reasonably attainable, and sometimes that's best served by doing more work that ultimately makes your patch simpler and leaves the code in a more maintainable position. Noticing that kind of opportunity is a large part of the point of getting code review from experienced project maintainers.
| @@ -219,6 +219,29 @@ extension UInt { | |||
| } | |||
|
|
|||
| /// A wrapper around a C `va_list` pointer. | |||
| #if arch(arm64) && os(Linux) | |||
There was a problem hiding this comment.
This is wrong. This applies to all AAPCS64 targets. Please change this to:
#if arch(arm64) && !(os(macOS) || os(iOS) || os(tvOS) || os(watchOS) || os(Windows))
There was a problem hiding this comment.
I made a counter-suggestion below that we should just add a custom platform-condition check for the definition kind of va_list.
There was a problem hiding this comment.
No, this correct. The purpose of this PR is to add support for the aarch64/Linux platform not all AAPCS64 targets.
| value = (__stack, __gr_top, __vr_top, __gr_off, __vr_off) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think that this should have an implementation of CustomDebugStringConvertible.
| @@ -90,6 +90,23 @@ internal let _registerSaveWords = _countGPRegisters + _countFPRegisters * _fpReg | |||
| internal let _countGPRegisters = 16 | |||
| @usableFromInline | |||
| internal let _registerSaveWords = _countGPRegisters | |||
|
|
|||
| #elseif arch(arm64) && os(Linux) | |||
There was a problem hiding this comment.
Similar to the condition above.
There was a problem hiding this comment.
As answered above.
| @usableFromInline | ||
| internal let _countFPRegisters = 8 | ||
| @usableFromInline | ||
| internal let _fpRegisterWords = 16 / MemoryLayout<Int>.size |
There was a problem hiding this comment.
I think that MemoryLayout<double>.size / MemoryLayout<Int>.size is better.
| @@ -498,6 +515,129 @@ final internal class _VaListBuilder { | |||
| Builtin.addressof(&self.header))) | |||
| } | |||
| } | |||
| #elseif arch(arm64) && os(Linux) | |||
There was a problem hiding this comment.
Similar to the other cases for the enabling.
There was a problem hiding this comment.
As answered above.
| internal func append(_ arg: CVarArg) { | ||
| var encoded = arg._cVarArgEncoding | ||
|
|
||
| if arg is _CVarArgPassedAsDouble |
There was a problem hiding this comment.
Can you hoist out the arg is _CVarArgPassedAsDouble?
|
|
||
| if arg is _CVarArgPassedAsDouble | ||
| && fpRegistersUsed < _countFPRegisters { | ||
| var startIndex = (fpRegistersUsed * _fpRegisterWords) |
There was a problem hiding this comment.
Should this not be offset by gpRegistersUsed like L554?
There was a problem hiding this comment.
No, this is correct.
| newStorage.moveInitialize(from: allocatedOldStorage, count: oldCount) | ||
| deallocStorage(wordCount: oldAllocated, storage: allocatedOldStorage) | ||
| } | ||
| } |
There was a problem hiding this comment.
Can't we use a ContiguousArray<Int> to back the storage and avoid all the allocation maintenance?
| let gr_top = vr_top + _countGPRegisters | ||
|
|
||
| return CVaListPointer(__stack: gr_top, __gr_top: gr_top, | ||
| __vr_top: vr_top, __gr_off: -64, __vr_off: -128) |
There was a problem hiding this comment.
Might be nice to compute the -64 and -128 using a let binding to make it explicitly how it is derived:
let VRAreaSize = MemoryLayout<double>.size * _countFPRegisters
let GRAreaSize = MemoryLayout<Int>.size * _countGPRegisters
| @@ -322,8 +322,11 @@ getSwiftStdlibType(const clang::TypedefNameDecl *D, | |||
| break; | |||
|
|
|||
| case MappedCTypeKind::VaList: | |||
| if (ClangTypeSize != ClangCtx.getTypeSize(ClangCtx.VoidPtrTy)) | |||
| return std::make_pair(Type(), ""); | |||
| if (ClangTypeSize != ClangCtx.getTypeSize(ClangCtx.VoidPtrTy)) { | |||
There was a problem hiding this comment.
Indentation here is off.
We shouldn't be doing a size-based validation here; we should be asking whether this type is equal to ClangCtx.getBuiltinVaListType(). If there are platforms where that doesn't work (because of slight pointer-size differences?) we should call them out specially instead of doing more work here.
There was a problem hiding this comment.
Doesn't my approach effectively accomplish that? Seems like both of us have the same underlying motivation though.
There was a problem hiding this comment.
Well, but your approach still hardcodes target differences that we don't really need to care about in the importer. All we really care about for portability is that the stdlib actually implements CVaListPointer correctly for all the different kinds of va_list, and that's easier to metaprogram directly based on Clang's existing knowledge of the target's va_list kind than it is to write a bunch of awkward platform conditions that essentially duplicate it.
| @@ -219,6 +219,29 @@ extension UInt { | |||
| } | |||
|
|
|||
| /// A wrapper around a C `va_list` pointer. | |||
| #if arch(arm64) && os(Linux) | |||
There was a problem hiding this comment.
We should really just add a new _va_list_kind #if condition. This is not nearly as complicated as you might think, and it doesn't need to be done elegantly because it will never, ever need to be made part of the documented language.
Just add a new enumerator to PlatformConditionKind, add a new case to getPlatformConditionKind in ParseIfConfig.cpp, and add a new case to LangOptions::checkPlatformConditionSupported which matches some strings up with the appropriate cases of getBultinVaListKind().
|
As I mentioned in #20863, the review comments here have not been addressed (despite them being marked as "resolved") and I don't see any agreement from the reviewers with your opinion that the other changes are not needed now. If we can't reach a consensus soon, I think this PR should be temporarily reverted until the review is completed. |
|
@bob-wilson I have re-opened the conversations that I had previously incorrectly marked as resolved. We now have a new PR - #21237 which addresses the comments of the reviewers of this PR. The new PR also adds support for other platforms on AArch64. @compnerd @rjmccall apologies if my replies have been a bit short, I have limited time I can volunteer on this project at the moment. I am sure this is the same for many others with end of year deadlines. I am hoping we can all reach a consensus to move forward with this PR unchanged and address the issues raised in the new PR - #21237 |
This refactoring uses large portions of the already existing code for x86_64 and s390 to implement AAPCS64 __VaListBuilder. The parts where each implementation differ are the x86_64 header, and the order of the general and vector registers. The changes also addresses many of the request from the reviewers in swiftlang#20862 which were not addressed originally, like the reuse of the already existing code, and the generalizations for the code to be useful for platforms different than Linux (Android, for example).
Use the already existing support for x86_64 and s390 to implement AAPCS64 __VaListBuilder. The parts where each implementation differ are the x86_64 header, and the order of the general and vector registers. This change brings in one commit the changes introduced in swiftlang#20862 and in swiftlang#21237.
Adds support for AArch64 variadics.
This is an updated version of #20708
Resolves SR-2239.
Previous issues preventing building of the
masterhave been fixed by defaulting to the "gold" linker.This has been updated in #20845
We are now seeing successful builds of the
masteron AArch64 Linux Ubuntu 16.04http://futurejones.xyz:8080/job/swift-master-aarch64/