Use builtins instead of shims in FloatingPointTypes. #3454

stephentyrone · 2016-07-11T21:57:49Z

What's in this pull request?

Replace shims for ceil, floor, round, etc. with Builtins to eliminate a level of indirection.

Resolved bug number: (SR-2045)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

jckarter · 2016-07-11T23:11:41Z

Don't the LLVM intrinsics have different semantics from the C functions in some cases? Are there differences we care about? (Not setting errno is for the better, at least.)

stephentyrone · 2016-07-11T23:18:50Z

@jckarter So they're all defined to do the right thing except for llvm.sqrt, which is annoyingly undef for non-zero negative inputs. Given that, how should we go about getting the instruction we actually want instead of a call for squareRoot?

jckarter · 2016-07-11T23:55:53Z

Dunno, is the backend smart enough to do the right thing if you write the test-and-br-or-select combo?

jckarter · 2016-07-12T00:08:04Z

Doesn't appear to do so for something like this, alas:

declare double @llvm.sqrt.f64(double %x)

define double @sqrtOrNaN(double %x) {
entry:
  %z = fcmp ult double %x, 0.0
  %b = call double @llvm.sqrt.f64(double %x)
  %c = select i1 %z, double 0x7fff000000000000, double %b
  ret double %c
}

But maybe we could ask the backend to recognize that sort of pattern.

stephentyrone · 2016-07-12T00:09:24Z

@jckarter Yeah, I would expect it not to. The backend knows how to lower an actual sqrt[f] call correctly, but (I don't think) we can use that in the standard lib, right?

jckarter · 2016-07-12T00:13:28Z

@stephentyrone Not without dirty tricks.

@gribozavr Would it be possible for the stub implementation to be static inline and call through to the real sqrt in the stubs header, something like this?

// Forward-declare sqrt to avoid including the real Darwin module.
double sqrt(double);

// Stub out a call to it.
static inline double _swift_stdlib_sqrt(double x) { return sqrt(x); }

jckarter · 2016-07-12T00:17:11Z

…though sqrt still couldn't be reduced to a machine instruction on platforms where the libc function sets errno or calls _matherr or has other nonstandard side effects (as in MSVC for instance), could it?

gribozavr · 2016-07-12T00:18:43Z

I'd rather not forward-declare sqrt(), since it would be imported by Clang importer into the global namespace.

stephentyrone · 2016-07-12T00:31:29Z

@jckarter @gribozavr Worked around using __builtin_sqrt. I believe this solves all the problems.

jckarter · 2016-07-12T02:05:02Z

SGTM.

stephentyrone · 2016-07-12T14:06:07Z

@swift-ci please test Linux.

stephentyrone · 2016-07-12T14:42:11Z

@jckarter @gribozavr @rjmccall Do you know what attributes we need on these static inlines to placate the linux build gods?

gribozavr · 2016-07-13T09:49:33Z

@stephentyrone This looks like a compiler bug to me.

gribozavr · 2016-07-13T09:52:21Z

stdlib/public/SwiftShims/LibcShims.h

@@ -84,35 +84,35 @@ __swift_uint32_t
 _swift_stdlib_cxx11_mt19937_uniform(__swift_uint32_t upper_bound);

 // Math library functions
-SWIFT_RUNTIME_STDLIB_INLINE
+SWIFT_RUNTIME_STDLIB_INTERFACE


I don't think you should need any attributes at all on these functions.

jckarter · 2016-07-13T16:49:45Z

Agree with @gribozavr, looks like SILGen's neglecting to emit something.

stephentyrone · 2016-07-13T19:08:45Z

Ok, I'll add a workaround for non-macOS once I have my current world-is-on-fire P1 dealt with.

stephentyrone · 2016-07-14T21:03:29Z

@swift-ci please test linux.

This reverts commit 999885f. This breaks the stdlib serialization tests: Assertion failed: (!hasSharedVisibility(F->getLinkage()) && "external declaration of SILFunction with shared visibility is not " "allowed"), function visitSILFunction, file /s/sptr/swift/lib/SIL/SILVerifier.cpp, line 3267.

atrick · 2016-07-15T16:02:47Z

Please see https://ci.swift.org/job/oss-swift_tools-RA_stdlib-RDA_long-test/507/

Assertion failed: (!hasSharedVisibility(F->getLinkage()) && "external declaration of SILFunction with shared visibility is not " "allowed"), function visitSILFunction, file /s/sptr/swift/lib/SIL/SILVerifier.cpp, line 3267.

While verifying SIL function @_swift_stdlib_sqrtf

Sorry, I don't know enough about the shims to fix it on the spot. But apparently SIL verifier cannot handle these static inline functions ??

stephentyrone · 2016-07-15T16:08:29Z

@atrick Why didn't we see this failure in the pre-submission test?

stephentyrone · 2016-07-15T16:14:11Z

Rather than reverting this, a much better fix would be to remove the #if defined __APPLE__ versions of the shims so we don't lose all the other performance wins.

gribozavr · 2016-07-15T16:14:35Z

Pre-submission test does not build the library in all modes, probably Release+DebugInfo+Assertions trips a bug in the compiler.

gribozavr · 2016-07-15T16:16:39Z

Rather than reverting this, a much better fix would be to remove the #if defined __APPLE__ versions of the shims so we don't lose all the other performance wins.

My concern is that if the compiler choked on one, then it is likely that others will also break in a similar way. If not in our tests, then in users' code.

stephentyrone · 2016-07-15T16:17:36Z

@gribozavr I don't understand your comment. There are no "others" for it to choke on. Those are the only static inlines.

gribozavr · 2016-07-15T16:26:15Z

I meant that the compiler didn't like _swift_stdlib_sqrtf. If we switch that one to use an out-of-line function (that's how I understood your suggestion), I think other functions (e.g., _swift_stdlib_remainder) are likely to trigger similar failures.

atrick · 2016-07-15T16:27:16Z

I don't think the presubmission tests run the "check-swift-long" does it? I was actually hitting this locally and it was blocking my work yesterday because I specifically needed to verify stdlib parsing.

stephentyrone · 2016-07-15T16:28:09Z

My suggestion is that we switch all six inlines to out-of-lines. That's what removing the #if defined __APPLE__ parts would do, without introducing large perf regressions for fma, floor, ceil, and trunc, which are some of the more useful math operations.

gribozavr · 2016-07-15T16:29:54Z

@stephentyrone Sorry, I misunderstood you then!

stephentyrone · 2016-07-15T17:04:41Z

Reworked as #3530 without the static inlines.

atrick · 2016-07-15T17:15:28Z

@stephentyrone Can you file a bug on the static inline problem referencing this PR?

stephentyrone · 2016-07-15T17:55:41Z

@atrick https://bugs.swift.org/browse/SR-2089. A bit vague since I don't actually understand the source of the error =)

Resolve conflicts with the `main` branch

stephentyrone added 2 commits July 11, 2016 17:55

Map .round onto builtins instead of shims for Float and Double.

289cdc1

Use builtins for Float80.round too.

10cff66

stephentyrone changed the title ~~Map .round onto builtins instead of shims for Float and Double.~~ Map .round onto builtins instead of shims. Jul 11, 2016

Might as well address lowering sqrt and fma while we're at it.

88952a1

Workaround the fact that llvm.sqrt is undef for negative.

cde32de

... and why not handle remainder exactly the same way?

848b86f

Semi-informed try at fixing visibility errors on Linux.

2d6b4ae

stephentyrone changed the title ~~Map .round onto builtins instead of shims.~~ Use builtins instead of shims in FloatingPointTypes. Jul 12, 2016

Add SWIFT_RUNTIME_STDLIB_INTERFACE to new static inlines.

9673937

gribozavr reviewed Jul 13, 2016
View reviewed changes

stephentyrone added 2 commits July 14, 2016 15:35

Revert to normal shims on non-Apple platforms for now.

39c5ce9

Properly namespace for non-apple shims.

286bd60

stephentyrone merged commit 999885f into swiftlang:master Jul 14, 2016

stephentyrone mentioned this pull request Jul 15, 2016

Intrinsics instead of shims #3530

Merged

1 task

stephentyrone deleted the slimmed-down-round branch July 15, 2016 17:04

kateinoigakukun pushed a commit to kateinoigakukun/swift that referenced this pull request Sep 5, 2021

Merge pull request swiftlang#3454 from swiftwasm/maxd/main-merge

3c90b17

Resolve conflicts with the `main` branch

stephentyrone mentioned this pull request Jul 11, 2016

[SR-2045] Optimization of FloatingPoint .round for Float/Double/Float80 #44654

Closed

Use builtins instead of shims in FloatingPointTypes. #3454

Use builtins instead of shims in FloatingPointTypes. #3454

Uh oh!

Conversation

stephentyrone commented Jul 11, 2016

What's in this pull request?

Resolved bug number: (SR-2045)

Uh oh!

jckarter commented Jul 11, 2016

Uh oh!

stephentyrone commented Jul 11, 2016

Uh oh!

jckarter commented Jul 11, 2016

Uh oh!

jckarter commented Jul 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentyrone commented Jul 12, 2016

Uh oh!

jckarter commented Jul 12, 2016

Uh oh!

jckarter commented Jul 12, 2016

Uh oh!

gribozavr commented Jul 12, 2016

Uh oh!

stephentyrone commented Jul 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jckarter commented Jul 12, 2016

Uh oh!

stephentyrone commented Jul 12, 2016

Uh oh!

stephentyrone commented Jul 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribozavr commented Jul 13, 2016

Uh oh!

gribozavr Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

jckarter commented Jul 13, 2016

Uh oh!

stephentyrone commented Jul 13, 2016

Uh oh!

stephentyrone commented Jul 14, 2016

Uh oh!

atrick commented Jul 15, 2016

Uh oh!

stephentyrone commented Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentyrone commented Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribozavr commented Jul 15, 2016

Uh oh!

gribozavr commented Jul 15, 2016

Uh oh!

stephentyrone commented Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribozavr commented Jul 15, 2016

Uh oh!

atrick commented Jul 15, 2016

Uh oh!

stephentyrone commented Jul 15, 2016

Uh oh!

gribozavr commented Jul 15, 2016

Uh oh!

stephentyrone commented Jul 15, 2016

Uh oh!

atrick commented Jul 15, 2016

Uh oh!

stephentyrone commented Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jckarter commented Jul 12, 2016 •

edited

Loading

stephentyrone commented Jul 12, 2016 •

edited

Loading

stephentyrone commented Jul 12, 2016 •

edited

Loading

stephentyrone commented Jul 15, 2016 •

edited

Loading

stephentyrone commented Jul 15, 2016 •

edited

Loading

stephentyrone commented Jul 15, 2016 •

edited

Loading

stephentyrone commented Jul 15, 2016 •

edited

Loading