[stdlib][SR-2239]: add support for AAPCS64 variadics #15174

KoVuInc · 2018-03-12T08:41:39Z

Support variadics on aarch64 aapcs64

KoVuInc · 2018-03-12T08:42:37Z

@swift-ci Please smoke test

compnerd · 2018-03-12T15:47:06Z

lib/ClangImporter/ImportDecl.cpp

+    if (ClangTypeSize != ClangCtx.getTypeSize(ClangCtx.VoidPtrTy)){
+       if(ClangCtx.getTargetInfo().getBuiltinVaListKind() != clang::TargetInfo::AArch64ABIBuiltinVaList)
+         return std::make_pair(Type(), "");
+    }


Please clang-format this bit.

compnerd · 2018-03-12T15:47:42Z

stdlib/public/core/CTypes.swift

@@ -227,6 +227,29 @@ extension UInt {
  }
 }

+#if arch(arm64) && !(os(macOS) || os(iOS) || os(tvOS) || os(watchOS))


This should also include Windows, as Windows ARM64 handles variadics similar to macOS.

compnerd · 2018-03-12T15:48:31Z

stdlib/public/core/VarArgs.swift

+@_versioned
+internal let _countFPRegisters = 8
+
+//128bit, 2 64bit word


Space after the comment leader

Hyphen between “128” and “bit” and between “64” and “bit”; noun-verb agreement requires “words,” not “word.” End the comment with punctuation.

compnerd · 2018-03-12T15:49:38Z

stdlib/public/core/VarArgs.swift

+
+//128bit, 2 64bit word
+@_versioned
+internal let _fpRegisterWords = 2


Might be nicer to express this as a sizeof(double) / sizeof(int) or something like that.

compnerd · 2018-03-12T15:49:59Z

stdlib/public/core/VarArgs.swift

+@_fixed_layout // FIXME(sil-serialize-all)
+@_versioned // FIXME(sil-serialize-all)
+final internal class _VaListBuilder {
+


Unnecessary newline

compnerd · 2018-03-12T15:50:15Z

stdlib/public/core/VarArgs.swift

@@ -467,6 +491,140 @@ final internal class _VaListBuilder {
  internal var storage: ContiguousArray<Int>
 }

+#elseif arch(arm64) && !(os(macOS) || os(iOS) || os(tvOS) || os(watchOS))
+
+


Unnecessary newline

compnerd · 2018-03-12T15:51:35Z

stdlib/public/core/VarArgs.swift

+            startIndex += 1
+          }
+          fpRegistersUsed += 1
+    }else if encoded.count == 1


space after }.

compnerd · 2018-03-12T15:52:06Z

stdlib/public/core/VarArgs.swift

+        var startIndex = ( _fpRegisterWords * _countFPRegisters) + gpRegistersUsed
+        storage[startIndex] = encoded[0]
+        gpRegistersUsed += 1
+    }else{


space around else

compnerd · 2018-03-12T15:55:02Z

@swift-ci please test

swift-ci · 2018-03-12T17:39:11Z

Build failed
Swift Test Linux Platform
Git Sha - 22d98a1

xwu

Some more code formatting comments.

xwu · 2018-03-12T22:29:44Z

stdlib/public/core/VarArgs.swift

+@_versioned
+internal let _countFPRegisters = 8
+
+//128bit, 2 64bit word


Hyphen between “128” and “bit” and between “64” and “bit”; noun-verb agreement requires “words,” not “word.” End the comment with punctuation.

xwu · 2018-03-12T22:30:18Z

stdlib/public/core/VarArgs.swift

+internal let _fpRegisterWords = 2
+
+@_versioned
+internal let _registerSaveWords = _countGPRegisters + (_countFPRegisters * _fpRegisterWords)


Please wrap to 80 characters.

xwu · 2018-03-12T22:32:11Z

stdlib/public/core/VarArgs.swift

+
+    if arg is _CVarArgPassedAsDouble
+      && fpRegistersUsed < _countFPRegisters {
+        var startIndex = (fpRegistersUsed * _fpRegisterWords)


Indenting should be increased by two spaces compared to “if”—you can choose to move the brace down to its own line when the conditional expression is multi-line, if that helps you.

xwu · 2018-03-12T22:32:36Z

stdlib/public/core/VarArgs.swift

+        gpRegistersUsed += 1
+    }else{
+        //arguments in stack slot
+        appendWords(encoded)


Likewise here, the indent should be increased by two and not four spaces.

xwu · 2018-03-12T22:33:22Z

stdlib/public/core/VarArgs.swift

+      allocated = max(newCount, allocated * 2)
+      let newStorage = allocStorage(wordCount: allocated)
+      storage = newStorage
+      // count is updated below


Please write comments in sentence case, with a beginning capital letter and a closing period.

xwu · 2018-03-12T22:34:20Z

stdlib/public/core/VarArgs.swift

+#elseif arch(arm64)
+
+// ARM IHI 0055B
+// va_list may refer to any paramenter may be in one of three memory locations:


Typo: parameter; missing word between “parameter” and “may”; needs wrapping to 80 characters.

KoVuInc · 2018-03-13T03:49:28Z

Just fix spacing, indentation and formatting.

compnerd · 2018-03-13T20:49:13Z

@swift-ci please test

swift-ci · 2018-03-13T20:49:39Z

Build failed
Swift Test OS X Platform
Git Sha - 22d98a1

compnerd · 2018-03-19T00:02:54Z

@swift-ci please test macOS platform

jasonm128 · 2018-03-23T16:46:48Z

I tried applying these changes as a patch to swift-4.1-DEVELOPMENT-SNAPSHOT-2018-03-05-a since that branch has many fewer test failures than master on Linux aarch64. The ClangImporter/ctypes_parse.swift test passes now but the stdlib/VarArgs.swift test still fails (although it does much better). It looks like handling of FP arguments is not correct:

/scratch/jasonm/swift-4.1-DEV-2018-03-05_AAPCS64-patch/swift/test/stdlib/VarArgs.swift:21:12: error: expected string not found in input
 // CHECK: The answer to life and everything is 42, 42, -42, 3.14
           ^
<stdin>:1:1: note: scanning from here
The answer to life and everything is 42, 42, -42, 0.000000
^

NB: I'm a swift newbie so I apologize if I'm doing something wrong here but I thought my input might help.

KoVuInc · 2018-03-24T09:08:47Z

@jasonm128
The VarArgs.swift should pass in the last patch. Just tested on aarch64 android.

jasonm128 · 2018-03-26T01:26:10Z

I can confirm that this latest version fixes the VarArgs.swift test on Linux aarch64 (Ubuntu 16.04).

However, since the whole sizeof(double)/sizeof(int) thing didn't work, I think it might be clearer to just set _fpRegisterWords = 2. This would match the style of the other architectures in this file. I'll defer to @compnerd on this though.

compnerd · 2018-04-03T00:31:19Z

@swift-ci please test

futurejones · 2018-06-20T01:08:38Z

Can we get this merged with the "swift-4.1-branch" and the "swift-4.2-branch"?

vielmetti · 2018-06-20T03:23:33Z

This PR will address a problem that's blocking a clean build by the new arm64 CI for Swift, referenced at swiftlang/swift-community-hosted-continuous-integration#10 .

vielmetti · 2018-06-27T14:30:42Z

@compnerd please take a look?

futurejones · 2018-07-10T14:42:43Z

@airspeedswift @jrose-apple can you help?
This issue is blocking all the builds on https://ci-external.swift.org/computer/ubuntu-16.04-aarch64/
What do we need to do to get this approved and merged?
Thanks.

jrose-apple

Oof, this has been open a while. I'd still hope to have someone more ABI-savvy than me review this, but here's some initial comments. (@compnerd only works on Swift in his free time, so we can't force him to participate.)

jrose-apple · 2018-07-10T16:25:35Z

stdlib/public/core/VarArgs.swift

+    let vr_top = storage + (_fpRegisterWords * _countFPRegisters)
+    let gr_top = vr_top + _countGPRegisters
+
+    return CVaListPointer(__stack: gr_top, __gr_top: gr_top,


__stack and __gr_top are not always the same for long argument lists, right?

jrose-apple · 2018-07-10T16:26:32Z

stdlib/public/core/VarArgs.swift

+
+@_fixed_layout // FIXME(sil-serialize-all)
+@_versioned // FIXME(sil-serialize-all)
+final internal class _VaListBuilder {


This seems heavily duplicated with the x86_64 / s390x implementation. Is it possible to merge them?

jrose-apple · 2018-07-10T16:27:55Z

stdlib/public/core/VarArgs.swift

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


In the AAPCS doc I see a 16-byte alignment for some of the fields. Is that accounted for elsewhere?

jrose-apple · 2018-07-10T16:28:46Z

stdlib/public/core/CTypes.swift

+                       __vr_off: Int32)
+
+  @_inlineable // FIXME(sil-serialize-all)
+  public // @testable


I don't see any direct tests of this initializer, but that's probably okay. That means you can leave it internal.

jrose-apple · 2018-07-10T16:29:28Z

stdlib/public/core/CTypes.swift

+#if arch(arm64) && !(os(macOS) || os(iOS) || os(tvOS) || os(watchOS) || os(Windows))
+@_fixed_layout
+public struct CVaListPointer {
+  @_versioned // FIXME(sil-serialize-all)


Nitpick: we now have non-underscored forms of _versioned and _inlineable (usableFromInline and 'inlinable`, respectively). I know this is an artifact of how long this patch has sat here, though!

futurejones · 2018-07-10T22:21:13Z

@jrose-apple thanks for looking at this.

futurejones · 2018-07-10T22:22:27Z

@KoVuInc @jasonm128 are you available to update this PR?

jasonm128 · 2018-07-11T03:31:21Z

@futurejones I'm willing to help but I'm not sure I'm able. I'm still new to Swift so I'm not familiar with a lot of the subtleties. I maintain an aarch64 system and have been trying to make Swift available for other users. I also don't have push access to the OP's repo. Let's see if @KoVuInc is available and I'll try to help if not.

KoVuInc · 2018-07-12T07:08:11Z

@futurejones @jasonm128 Sure, I'll spend sometime to update this PR.

compnerd · 2018-07-17T18:29:13Z

lib/ClangImporter/ImportDecl.cpp

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


I think that this might be easier to read as:

if (ClangCtx.getTargetInfo().getBuiltinVaListKind() == clang::TargetInfo::AArch64ABIBuiltinVaList) break; if (ClangTypeSize == ClangCtx.getTypeSize(ClangCtx.VoidPtrTy)) break; return std::make_pair(Type(), "");

Should we just switch over getBuiltinVaListKind() instead of considering the size of the type at all?

I think the original intent was to make sure it was layout-compatible with Swift.CVaListPointer, which is a simpler proposition, but that would probably work too.

@jrose-apple, I think that switching over getBuiltinVaListKind accomplishes that and much more explicit. Just because they they have the same size doesn't necessarily guarantee layout compatibility.

compnerd · 2018-07-17T18:32:18Z

stdlib/public/core/VarArgs.swift

+  internal func append(_ arg: CVarArg) {
+    var encoded = arg._cVarArgEncoding
+
+    if arg is _CVarArgPassedAsDouble


Can you hoist out arg is _CVarArgPassedAsDouble into a variable?

let isDouble = arg is _CVarArgPassedAsDouble

CodaFi · 2019-11-21T22:04:32Z

@KoVuInc It's been a while and there hasn't been any activity here. The SR has been released back to the community. Thank you for everything you did here. I'm sure some other contributor will find this patch a very helpful place to start.

stdlib: add support for AAPCS64 variadics

22d98a1

KoVuInc changed the title ~~stdlib: add support for AAPCS64 variadics~~ [stdlib][SR-2239]: add support for AAPCS64 variadics Mar 12, 2018

compnerd reviewed Mar 12, 2018

View reviewed changes

xwu reviewed Mar 12, 2018

View reviewed changes

KoVuInc force-pushed the master branch from 31e9a12 to ddac6c7 Compare March 13, 2018 03:35

Fix spacing, indentation, capitalization

304d8ca

KoVuInc force-pushed the master branch from ddac6c7 to 304d8ca Compare March 13, 2018 03:42

[stdlib] VarArgs aarch64 FP/SIMD register shall be 16bytes

089e1b9

This was referenced May 4, 2018

WIP/IRGen: PCS for ARM VFP CC does not correctly handle HFAs #14304

Open

[WIP] stdlib: add support for AAPCS64 variadics #12623

Closed

jasonm128 mentioned this pull request May 9, 2018

Add patches and support for 64-bit ARM (aarch64) uraimo/buildSwiftOnARM#6

Merged

futurejones mentioned this pull request Jun 20, 2018

Add aarch64 node swiftlang/swift-community-hosted-continuous-integration#10

Merged

jrose-apple reviewed Jul 10, 2018

View reviewed changes

jrose-apple requested a review from rjmccall July 10, 2018 16:30

compnerd reviewed Jul 17, 2018

View reviewed changes

futurejones mentioned this pull request Nov 23, 2018

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

Merged

CodaFi closed this Nov 21, 2019

futurejones mentioned this pull request Jun 21, 2018

[SR-2239] CVaListPointer does not gets converted to va_list on aarch64 #44846

Open

[stdlib][SR-2239]: add support for AAPCS64 variadics #15174

[stdlib][SR-2239]: add support for AAPCS64 variadics #15174

Uh oh!

Conversation

KoVuInc commented Mar 12, 2018

Uh oh!

KoVuInc commented Mar 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Mar 12, 2018

Uh oh!

swift-ci commented Mar 12, 2018

Uh oh!

xwu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwu Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KoVuInc commented Mar 13, 2018

Uh oh!

compnerd commented Mar 13, 2018

Uh oh!

swift-ci commented Mar 13, 2018

Uh oh!

compnerd commented Mar 19, 2018

Uh oh!

jasonm128 commented Mar 23, 2018

Uh oh!

KoVuInc commented Mar 24, 2018

Uh oh!

jasonm128 commented Mar 26, 2018

Uh oh!

compnerd commented Apr 3, 2018

Uh oh!

futurejones commented Jun 20, 2018

Uh oh!

vielmetti commented Jun 20, 2018

Uh oh!

vielmetti commented Jun 27, 2018

Uh oh!

futurejones commented Jul 10, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwu Mar 12, 2018 •

edited

Loading