Add `Builtin.isConcrete<T>(T.Type) -> Int1` #26466

nvzqz · 2019-08-02T18:37:07Z

Returns true if T.Type is known to refer to a concrete type during IRGen. Otherwise, if still generic, then it is lowered to false right before IRGen in IRGenPrepare. This allows for the optimizer to specialize this at -O and eliminate conditional code.

This also Includes:

Swift._isConcrete<T>(T.Type) -> Bool wrapper function.
tests on the [un]optimized SIL generated from calling isConcrete on generic and concrete types in -O[none] builds.

This is prior work to adding Builtin.generic_add<T>(T, T) -> T and friends for SIMD vector types.

stephentyrone · 2019-08-02T18:38:56Z

Thanks @nvzqz! I'll take a look later today.

stephentyrone · 2019-08-02T18:39:05Z

@swift-ci please smoke test

jckarter · 2019-08-02T18:43:28Z

Our existing precedent for this is to name these sorts of things canBe. Check out canBeClass for an example. canBeClass also returns a tristate, because it ended up being valuable to know whether the answer is "definitely no", "definitely yes", or "maybe".

stephentyrone · 2019-08-02T18:45:55Z

Our existing precedent for this is to name these sorts of things canBe. Check out canBeClass for an example. canBeClass also returns a tristate, because it ended up being valuable to know whether the answer is "definitely no", "definitely yes", or "maybe".

Is this actually tri-state logically though, or do we always know by the time we reach IRGen?

jckarter · 2019-08-02T18:46:01Z

If you look at how TypeBase::canBeClass is implemented in the SILCombinerBuiltinVisitors, on top of TypeTraitResult TypeBase::canBeClass(), there should also be a bunch of existing code you can factor out and share between these two traits.

nvzqz · 2019-08-02T18:46:05Z

@jckarter that name does seem better. I'm not sure if there's a way of expressing this as a tristate.

jckarter · 2019-08-02T18:47:00Z

@nvzqz Maybe not in this case, but I think by following the pattern set by canBeClass, you could leverage a bunch of existing implementation, and make it easier to add more traits like this in the future.

jckarter · 2019-08-02T18:58:44Z

The way that canBeClass works is that the optimizer immediately constant-folds to Is or IsNot when the trait method gives a definite answer, but leaves the builtin in place for CanBe. IRGen then lowers the builtin to the constant value CanBe if it hasn't already been lowered away. In the case of "is-concrete", the only interesting cases are Is and CanBe, I guess, but I think you still want to share the same approach, because this allows the builtin to constant-fold early, thereby allowing SIL-level optimizations to take advantage of the constant folding, without preventing it from also folding later as more information gets exposed.

nvzqz · 2019-08-03T01:30:24Z

@swift-ci please smoke test

stephentyrone · 2019-08-03T02:30:42Z

@swift-ci please smoke test

stdlib/public/core/Builtin.swift

stephentyrone

LGTM, modulo @jckarter's questions about naming and reuse of existing code.

nvzqz · 2019-08-05T17:07:44Z

After thinking about it some more, I'm not sure if a tri-state naming convention makes sense for this. I think that isConcrete already says what it does on the tin in that it expresses how the compiler sees the metatype in a given situation and configuration (e.g. -O and -Onone).

@jckarter I've attempted a different implementation that is done at the same stage as canBeClass but still using BUILTIN_MISC_OPERATION and returning a boolean. However, it's not producing the desired results and the compiler produces a segmentation fault in release builds. See nvzqz@010b6e3, branch.

See stack dump

0.	Program arguments: /Users/nvzqz/dev/swift-lang/build/Ninja-ReleaseAssert+stdlib-DebugAssert/swift-macosx-x86_64/bin/swiftc
	-frontend -target x86_64-apple-macosx10.9
	-module-cache-path /Users/nvzqz/dev/swift-lang/build/Ninja-ReleaseAssert+stdlib-DebugAssert/swift-macosx-x86_64/swift-test-results/x86_64-apple-macosx10.9/clang-module-cache
	-sdk /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk
	-swift-version 4 -typo-correction-limit 10
	-module-name constant_propagation_stdlib
	/Users/nvzqz/dev/swift-lang/swift/test/SILOptimizer/constant_propagation_stdlib.swift
	-parse-stdlib -emit-ir -o - -O
1.	Swift version 5.1-dev (LLVM 8d110eebee, Swift 74559a07d8)
2.	While running pass #71 SILFunctionTransform "SILCombine" on SILFunction "@$s27constant_propagation_stdlib15isConcrete_trueyBi1_AA5MyIntVF".
 for 'isConcrete_true(_:)' (at /Users/nvzqz/dev/swift-lang/swift/test/SILOptimizer/constant_propagation_stdlib.swift:19:8)
0  swiftc                   0x000000010d1b9155 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
1  swiftc                   0x000000010d1b8198 llvm::sys::RunSignalHandlers() + 248
2  swiftc                   0x000000010d1b9748 SignalHandler(int) + 264
3  libsystem_platform.dylib 0x00007fff76a16b5d _sigtramp + 29
4  libsystem_platform.dylib 000000000000000000 _sigtramp + 2304677056
5  swiftc                   0x0000000109b8ce52 swift::SILCombiner::runOnFunction(swift::SILFunction&) + 258
6  swiftc                   0x0000000109b8dc11 (anonymous namespace)::SILCombine::run() + 561
7  swiftc                   0x0000000109b7884f swift::SILPassManager::runPassOnFunction(unsigned int, swift::SILFunction*) + 1711
8  swiftc                   0x0000000109b7958f swift::SILPassManager::runFunctionPasses(unsigned int, unsigned int) + 1279
9  swiftc                   0x0000000109b7a43e swift::SILPassManager::execute() + 606
10 swiftc                   0x00000001097578db swift::SILPassManager::executePassPipelinePlan(swift::SILPassPipelinePlan const&) + 187
11 swiftc                   0x0000000109b81a55 swift::runSILOptimizationPasses(swift::SILModule&) + 117
12 swiftc                   0x0000000109816901 swift::CompilerInstance::performSILProcessing(swift::SILModule*, swift::UnifiedStatsReporter*) + 977
13 swiftc                   0x000000010960738c performCompile(swift::CompilerInstance&, swift::CompilerInvocation&, llvm::ArrayRef<char const*>, int&, swift::FrontendObserver*, swift::UnifiedStatsReporter*) + 11564
14 swiftc                   0x0000000109603730 swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 3024
15 swiftc                   0x00000001095ab309 main + 729
16 libdyld.dylib            0x00007fff7682b3d5 start + 1
17 libdyld.dylib            0x0000000000000014 start + 2306690112
[1]    93337 segmentation fault   -frontend -target x86_64-apple-macosx10.9 -module-cache-path  -sdk   4  10

I'm of the opinion that the current changes are sufficient for now and that we should defer optimizing it or reusing the code for canBeClass. I'll see if this still happens when isConcrete does use a tri state via the same route as canBeClass; i.e. use BUILTIN_TYPE_TRAIT_OPERATION.

gottesmm · 2019-08-05T19:46:23Z

Just as an exploration of the state space, consider the following:

Imagine that we had a builtin called canBeConcrete. It returns either always, never, sometimes (i.e. a tri-state).

If we have a concrete type, easily the builtin must return always since once a type is concrete we can not further specialize it.

What about generic types? Easily, there are two possibilities:

If we are still allowing for further specialization, we want to return maybe. This ensures that we do not eliminate the builtin too early.
If we somehow have the knowledge that no matter specialization will occur (for whatever reason), then we can lower to never.

The key thing to notice is that the 2nd bullet point via our semantics is IRGen time/Lowered SIL. That might be the way forward for this in a way that keeps the logic of when to fold this in a utility that the various code can invoke.

Thats all I got.

NOTE: If we were to embed these semantics if we don't assert that the specialization machinery never runs in Lowered SIL, we would necessarily have to do so in order to be careful and prevent mistakes.

jckarter · 2019-08-06T17:58:29Z

@nvzqz I commented on the linked commit. You should not be erasing the instruction from inside the SILCombiner method, since that will mess up SILCombiner's state. It'll clean up for you. You'd also need to implement the IRGen fallback implementation that emits false when the builtin still exists at IRGen time. With those changes, you should get the same benefits as canBeClass even if it isn't literally implemented the same. I don't care too much about the naming, but I think having an idiosyncratic name does help call attention to the fact that these have unusual compile-time-state behavior.

nvzqz · 2019-08-14T04:25:42Z

I walked through my implementations of @jckarter's suggestions with @atrick and found places where it was ill-formed. The concerns I expressed to @airspeedswift and @gottesmm previously with regard to eager evaluation (always emitting false/0) were apparently an artifact of this.

nvzqz · 2019-08-14T16:21:18Z

@swift-ci please smoke test

test/stdlib/Builtins.swift

stdlib/public/core/Builtin.swift

lib/SILOptimizer/Utils/ConstantFolding.cpp

test/stdlib/Builtins.swift

stephentyrone · 2019-08-15T18:03:10Z

stdlib/public/core/Builtin.swift

+/// this returns `false` if the check has failed.
+///
+/// Note that there may be cases in which, despite `T` being concrete at some
+/// point in the caller chain, this function will return `false`.


Maybe add that it will never return a false positive.

airspeedswift · 2019-08-15T18:53:23Z

@swift-ci please smoke test

test/IRGen/builtin_isConcrete.sil

nvzqz · 2019-08-20T18:20:11Z

@swift-ci please smoke test

nvzqz · 2019-08-20T21:04:22Z

@swift-ci please smoke test

test/SILOptimizer/constant_propagation_stdlib.swift

stephentyrone · 2019-08-27T21:53:43Z

My concerns are resolved, but please check with @atrick to make sure he's satisfied with the updated SIL tests.

Returns `true` if `T.Type` is known to refer to a concrete type. The implementation allows for the optimizer to specialize this at -O and eliminate conditional code. Includes `Swift._isConcrete<T>(T.Type) -> Bool` wrapper function.

The `Builtin.isConcrete` function is apparently sensitive to inlining in unoptimized builds.

Returns `false` otherwise in Debug builds because it is generic.

When calling `Builtin.isConcrete` in SIL, the provided type parameter should be sufficient.

nvzqz · 2019-08-27T23:59:07Z

@atrick the SIL tests have been addressed as well as the concerns around constantFoldIsConcrete being called twice. 🙂

airspeedswift · 2019-08-28T21:40:04Z

@atrick you good to go with this?

airspeedswift · 2019-08-28T21:40:52Z

@swift-ci please smoke test

atrick

I see the unreachable code in constandFoldBuiltin was removed. And the .sil test cases look reasonable. Thanks.

nvzqz · 2019-08-28T23:49:45Z

@swift-ci please test and merge

nvzqz requested review from gottesmm and stephentyrone August 2, 2019 18:37

stephentyrone requested a review from airspeedswift August 2, 2019 18:37

stephentyrone reviewed Aug 5, 2019

View reviewed changes

stdlib/public/core/Builtin.swift Outdated Show resolved Hide resolved

stephentyrone approved these changes Aug 5, 2019

View reviewed changes

nvzqz force-pushed the is_concrete_builtin branch from d184918 to 45777bc Compare August 14, 2019 04:24

nvzqz force-pushed the is_concrete_builtin branch from 45777bc to 44cc730 Compare August 14, 2019 04:41

stephentyrone reviewed Aug 14, 2019

View reviewed changes

test/stdlib/Builtins.swift Show resolved Hide resolved

stephentyrone reviewed Aug 14, 2019

View reviewed changes

stdlib/public/core/Builtin.swift Outdated Show resolved Hide resolved

stephentyrone reviewed Aug 14, 2019

View reviewed changes

lib/SILOptimizer/Utils/ConstantFolding.cpp Outdated Show resolved Hide resolved

stephentyrone self-requested a review August 15, 2019 12:10

stephentyrone requested changes Aug 15, 2019

View reviewed changes

test/stdlib/Builtins.swift Show resolved Hide resolved

stephentyrone reviewed Aug 15, 2019

View reviewed changes

nvzqz mentioned this pull request Aug 16, 2019

Optimize performance of SIMD binary operations via polymorphic builtins #26699

Closed

nvzqz commented Aug 20, 2019

View reviewed changes

test/IRGen/builtin_isConcrete.sil Outdated Show resolved Hide resolved

nvzqz requested review from stephentyrone and atrick August 20, 2019 20:11

stephentyrone reviewed Aug 24, 2019

View reviewed changes

test/SILOptimizer/constant_propagation_stdlib.swift Show resolved Hide resolved

nvzqz force-pushed the is_concrete_builtin branch from a3e180c to a73e146 Compare August 27, 2019 20:40

nvzqz requested a review from stephentyrone August 27, 2019 20:56

stephentyrone approved these changes Aug 27, 2019

View reviewed changes

nvzqz added 12 commits August 27, 2019 16:51

Add Builtin.isConcrete<T>(T.Type) -> Int1

ba0612f

Returns `true` if `T.Type` is known to refer to a concrete type. The implementation allows for the optimizer to specialize this at -O and eliminate conditional code. Includes `Swift._isConcrete<T>(T.Type) -> Bool` wrapper function.

Never inline isConcrete_false test

da14b83

The `Builtin.isConcrete` function is apparently sensitive to inlining in unoptimized builds.

Fix variable reference in ConstantFolding phase

c4cd641

Elaborate in docs for Swift._isConcrete

890409e

Remove Builtin.isConcrete IRGenPrepare SIL test

b35559e

Add IRGen SIL test for Builtin.isConcrete

6fd7db8

Add visibility specifiers to isConcrete IRGen test

34eccf5

Annotate Swift._isConcrete with @_transparent

6e542bf

Returns `false` otherwise in Debug builds because it is generic.

Add comment on required isConcrete specialization

a1f3cb9

When calling `Builtin.isConcrete` in SIL, the provided type parameter should be sufficient.

Add SIL combine test for Builtin.isConcrete

ff7542a

Generalize constant_propagation_stdlib SIL tests

e6ca03d

Remove unused call to constantFoldIsConcrete

931ab46

nvzqz force-pushed the is_concrete_builtin branch from c649378 to 931ab46 Compare August 27, 2019 23:51

airspeedswift removed their request for review August 28, 2019 21:41

atrick approved these changes Aug 28, 2019

View reviewed changes

swift-ci merged commit b8f42d6 into swiftlang:master Aug 29, 2019

Add Builtin.isConcrete<T>(T.Type) -> Int1 #26466

Add Builtin.isConcrete<T>(T.Type) -> Int1 #26466

Uh oh!

Conversation

nvzqz commented Aug 2, 2019

Uh oh!

stephentyrone commented Aug 2, 2019

Uh oh!

stephentyrone commented Aug 2, 2019

Uh oh!

jckarter commented Aug 2, 2019

Uh oh!

stephentyrone commented Aug 2, 2019

Uh oh!

jckarter commented Aug 2, 2019

Uh oh!

nvzqz commented Aug 2, 2019

Uh oh!

jckarter commented Aug 2, 2019

Uh oh!

jckarter commented Aug 2, 2019

Uh oh!

nvzqz commented Aug 3, 2019

Uh oh!

stephentyrone commented Aug 3, 2019

Uh oh!

Uh oh!

stephentyrone left a comment

Choose a reason for hiding this comment

Uh oh!

nvzqz commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gottesmm commented Aug 5, 2019

Uh oh!

jckarter commented Aug 6, 2019

Uh oh!

nvzqz commented Aug 14, 2019

Uh oh!

nvzqz commented Aug 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentyrone Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

airspeedswift commented Aug 15, 2019

Uh oh!

Uh oh!

nvzqz commented Aug 20, 2019

Uh oh!

nvzqz commented Aug 20, 2019

Uh oh!

Uh oh!

stephentyrone commented Aug 27, 2019

Uh oh!

nvzqz commented Aug 27, 2019

Uh oh!

airspeedswift commented Aug 28, 2019

Uh oh!

airspeedswift commented Aug 28, 2019

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

nvzqz commented Aug 28, 2019

Uh oh!

Uh oh!

Add `Builtin.isConcrete<T>(T.Type) -> Int1` #26466

Add `Builtin.isConcrete<T>(T.Type) -> Int1` #26466

nvzqz commented Aug 5, 2019 •

edited

Loading