Skip to content

[stdlib][SR-2239]: add support for AArch64 variadics #20862

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 29, 2018
Merged

[stdlib][SR-2239]: add support for AArch64 variadics #20862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't my approach effectively accomplish that? Seems like both of us have the same underlying motivation though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (ClangCtx.getTargetInfo().getBuiltinVaListKind() !=
clang::TargetInfo::AArch64ABIBuiltinVaList)
return std::make_pair(Type(), "");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(), "");
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

break;

case MappedCTypeKind::ObjCBool:
Expand Down
25 changes: 25 additions & 0 deletions stdlib/public/core/CTypes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,29 @@ extension UInt {
}

/// A wrapper around a C `va_list` pointer.
#if arch(arm64) && os(Linux)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a counter-suggestion below that we should just add a custom platform-condition check for the definition kind of va_list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this correct. The purpose of this PR is to add support for the aarch64/Linux platform not all AAPCS64 targets.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

@_fixed_layout
public struct CVaListPointer {
@usableFromInline // unsafe-performance
internal var value: (__stack: UnsafeMutablePointer<Int>?,
__gr_top: UnsafeMutablePointer<Int>?,
__vr_top: UnsafeMutablePointer<Int>?,
__gr_off: Int32,
__vr_off: Int32)

@inlinable // unsafe-performance
public // @testable
init(__stack: UnsafeMutablePointer<Int>?,
__gr_top: UnsafeMutablePointer<Int>?,
__vr_top: UnsafeMutablePointer<Int>?,
__gr_off: Int32,
__vr_off: Int32) {
value = (__stack, __gr_top, __vr_top, __gr_off, __vr_off)
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should have an implementation of CustomDebugStringConvertible.

#else

@_fixed_layout
public struct CVaListPointer {
@usableFromInline // unsafe-performance
Expand All @@ -238,6 +261,8 @@ extension CVaListPointer : CustomDebugStringConvertible {
}
}

#endif

@inlinable
internal func _memcpy(
dest destination: UnsafeMutableRawPointer,
Expand Down
140 changes: 140 additions & 0 deletions stdlib/public/core/VarArgs.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,23 @@ internal let _registerSaveWords = _countGPRegisters + _countFPRegisters * _fpReg
internal let _countGPRegisters = 16
@usableFromInline
internal let _registerSaveWords = _countGPRegisters

#elseif arch(arm64) && os(Linux)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the condition above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As answered above.

// ARM Procedure Call Standard for aarch64. (IHI0055B)
// The va_list type may refer to any parameter in a parameter list may be in one
// of three memory locations depending on its type and position in the argument
// list :
// 1. GP register save area x0 - x7
// 2. 128-bit FP/SIMD register save area q0 - q7
// 3. Stack argument area
@usableFromInline
internal let _countGPRegisters = 8
@usableFromInline
internal let _countFPRegisters = 8
@usableFromInline
internal let _fpRegisterWords = 16 / MemoryLayout<Int>.size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that MemoryLayout<double>.size / MemoryLayout<Int>.size is better.

@usableFromInline
internal let _registerSaveWords = _countGPRegisters + (_countFPRegisters * _fpRegisterWords)
#endif

#if arch(s390x)
Expand Down Expand Up @@ -498,6 +515,129 @@ final internal class _VaListBuilder {
Builtin.addressof(&self.header)))
}
}
#elseif arch(arm64) && os(Linux)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the other cases for the enabling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As answered above.


@_fixed_layout // FIXME(sil-serialize-all)
@usableFromInline // FIXME(sil-serialize-all)
final internal class _VaListBuilder {
@usableFromInline // FIXME(sil-serialize-all)
internal init() {
// Prepare the register save area.
allocated = _registerSaveWords
storage = allocStorage(wordCount: allocated)
// Append stack arguments after register save area.
count = allocated
}

@usableFromInline // FIXME(sil-serialize-all)
deinit {
if let allocatedStorage = storage {
deallocStorage(wordCount: allocated, storage: allocatedStorage)
}
}

@usableFromInline // FIXME(sil-serialize-all)
internal func append(_ arg: CVarArg) {
var encoded = arg._cVarArgEncoding

if arg is _CVarArgPassedAsDouble
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you hoist out the arg is _CVarArgPassedAsDouble?

&& fpRegistersUsed < _countFPRegisters {
var startIndex = (fpRegistersUsed * _fpRegisterWords)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be offset by gpRegistersUsed like L554?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is correct.

for w in encoded {
storage[startIndex] = w
startIndex += 1
}
fpRegistersUsed += 1
} else if encoded.count == 1
&& !(arg is _CVarArgPassedAsDouble)
&& gpRegistersUsed < _countGPRegisters {
var startIndex = ( _fpRegisterWords * _countFPRegisters) + gpRegistersUsed
storage[startIndex] = encoded[0]
gpRegistersUsed += 1
} else {
// Arguments in stack slot.
appendWords(encoded)
}
}

@usableFromInline // FIXME(sil-serialize-all)
internal func va_list() -> CVaListPointer {
let vr_top = storage + (_fpRegisterWords * _countFPRegisters)
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@usableFromInline // FIXME(sil-serialize-all)
internal func appendWords(_ words: [Int]) {
let newCount = count + words.count
if newCount > allocated {
let oldAllocated = allocated
let oldStorage = storage
let oldCount = count

allocated = max(newCount, allocated * 2)
let newStorage = allocStorage(wordCount: allocated)
storage = newStorage
// Count is updated below.
if let allocatedOldStorage = oldStorage {
newStorage.moveInitialize(from: allocatedOldStorage, count: oldCount)
deallocStorage(wordCount: oldAllocated, storage: allocatedOldStorage)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use a ContiguousArray<Int> to back the storage and avoid all the allocation maintenance?


let allocatedStorage = storage!
for word in words {
allocatedStorage[count] = word
count += 1
}
}

@usableFromInline // FIXME(sil-serialize-all)
internal func rawSizeAndAlignment(
_ wordCount: Int
) -> (Builtin.Word, Builtin.Word) {
return ((wordCount * MemoryLayout<Int>.stride)._builtinWordValue,
requiredAlignmentInBytes._builtinWordValue)
}

@usableFromInline // FIXME(sil-serialize-all)
internal func allocStorage(wordCount: Int) -> UnsafeMutablePointer<Int> {
let (rawSize, rawAlignment) = rawSizeAndAlignment(wordCount)
let rawStorage = Builtin.allocRaw(rawSize, rawAlignment)
return UnsafeMutablePointer<Int>(rawStorage)
}

@usableFromInline // FIXME(sil-serialize-all)
internal func deallocStorage(
wordCount: Int, storage: UnsafeMutablePointer<Int>
) {
let (rawSize, rawAlignment) = rawSizeAndAlignment(wordCount)
Builtin.deallocRaw(storage._rawValue, rawSize, rawAlignment)
}

@usableFromInline // FIXME(sil-serialize-all)
internal let requiredAlignmentInBytes = MemoryLayout<Double>.alignment

@usableFromInline // FIXME(sil-serialize-all)
internal var count = 0

@usableFromInline // FIXME(sil-serialize-all)
internal var allocated = 0

@usableFromInline // FIXME(sil-serialize-all)
internal var storage: UnsafeMutablePointer<Int>!

@usableFromInline // FIXME(sil-serialize-all)
internal var gpRegistersUsed = 0

@usableFromInline // FIXME(sil-serialize-all)
internal var fpRegistersUsed = 0

@usableFromInline // FIXME(sil-serialize-all)
internal var overflowWordsUsed = 0
}

#else

Expand Down