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.

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

Reduce array abstraction on apple platforms dealing with literals #13665

Merged
merged 5 commits into from Jan 2, 2018

Conversation

Projects
None yet
6 participants
@lattner
Collaborator

lattner commented Jan 2, 2018

Part of the ongoing quest to reduce swift array literal abstraction
penalties: make the SIL optimizer able to eliminate bridging overhead
when dealing with array literals.

Introduce a new classify_bridge_object SIL instruction to handle the
logic of extracting platform specific bits from a Builtin.BridgeObject
value that indicate whether it contains a ObjC tagged pointer object,
or a normal ObjC object. This allows the SIL optimizer to eliminate
these, which allows constant folding a ton of code. On the example
added to test/SILOptimizer/static_arrays.swift, this results in 4x
less SIL code, and also leads to a lot more commonality between linux
and apple platform codegen when passing an array literal.

This also introduces a couple of SIL combines for patterns that occur
in the array literal passing case.

This is the same as PR13659 but rebased on top of master.

lattner added some commits Jan 1, 2018

Reduce array abstraction on apple platforms dealing with literals
Part of the ongoing quest to reduce swift array literal abstraction
penalties: make the SIL optimizer able to eliminate bridging overhead
 when dealing with array literals.

Introduce a new classify_bridge_object SIL instruction to handle the
logic of extracting platform specific bits from a Builtin.BridgeObject
value that indicate whether it contains a ObjC tagged pointer object,
or a normal ObjC object. This allows the SIL optimizer to eliminate
these, which allows constant folding a ton of code. On the example
added to test/SILOptimizer/static_arrays.swift, this results in 4x
less SIL code, and also leads to a lot more commonality between linux
and apple platform codegen when passing an array literal.

This also introduces a couple of SIL combines for patterns that occur
in the array literal passing case.
Tweak testcases to work with variants of stdlib build. Super sad that
we don't reliably get stack allocations... yet.

@lattner lattner requested review from gottesmm, rjmccall and eeckstein Jan 2, 2018

@lattner

This comment has been minimized.

Collaborator

lattner commented Jan 2, 2018

@swift-ci please test

1 similar comment
@lattner

This comment has been minimized.

Collaborator

lattner commented Jan 2, 2018

@swift-ci please test

@lattner

This comment has been minimized.

Collaborator

lattner commented Jan 2, 2018

I realize that the motivation for this may not be entirely obvious. The point of this is to allow the SIL optimizer to reason about the magic bits that are mangled into the Builtin.BridgeObject values, instead of having them be extracted with and's of magic constants that are unknown to the optimizer (only IRGen does and should have access to this info). Here is one example of a before and after this patch:

https://gist.github.com/lattner/aa5cdefb9d9fe2b9f3508619e20b5225

I recognize that it is theoretically possible that lazy bridging will go away. However, until and if that happens, we should take this patch and continue providing more abstraction for the bridge representation.

@lattner

This comment has been minimized.

Collaborator

lattner commented Jan 2, 2018

@swift-ci please test

2 similar comments
@lattner

This comment has been minimized.

Collaborator

lattner commented Jan 2, 2018

@swift-ci please test

@lattner

This comment has been minimized.

Collaborator

lattner commented Jan 2, 2018

@swift-ci please test

@eeckstein

very nice! In general lgtm.

some comments: I think it makes sense to clean up Builtin.swift a bit - to use the new builtin and remove some functions/variables which are not needed anymore now.

It would be nice to get rid of the bridge_object_to_word instruction at all. But afaik, it is still needed at least for getting spare bits out of the array buffer pointer (the needsElementTypeCheck flag). Either we extend classify_bridge_object returns to return this spare bit as well or we'll add another instruction. But we can do this in a follow-up work.

sil-instruction ::= 'classify_bridge_object' sil-operand
%1 = classify_bridge_object %0 : $Builtin.BridgeObject
// %1 will be of type (Builtin.Int1, Builtin.Int1)

This comment has been minimized.

@eeckstein

eeckstein Jan 2, 2018

Member

SIL now supports multiple return values. This would simplify the representation, compared to going through a tuple:

(%1, %2) = classify_bridge_object %0 : $Builtin.BridgeObject
// %1 and %2 will be of type Builtin.Int1

This comment has been minimized.

@lattner

lattner Jan 2, 2018

Collaborator

I took a look at that, but it requires a ton of boilerplate to define a multi-result instruction (and there are only a few of them that do). If you feel strongly about this, I'd prefer to land this, then work on fixing the core representation of multi-result instructions in a follow-on patch, and adopt it later.

This comment has been minimized.

@rjmccall

rjmccall Jan 2, 2018

Member

I'd be interested to know what you feel is broken about multi-result instructions. I tried several different designs and concluded that there are just inherent trade-offs that have to be made here. I chose to make working with single-result instructions easier at the cost of adding boilerplate to multi-result instructions. If you have a concrete alternative design in mind, I'd love to hear it.

// CHECK-LABEL: sil @{{.*}}passArrayOfClasses
// CHECK: bb0(%0 : $SwiftClass, %1 : $SwiftClass, %2 : $SwiftClass):
// CHECK-NOT: bb1(
// CHECK: alloc_ref {{.*}}[tail_elems $SwiftClass *

This comment has been minimized.

@eeckstein

eeckstein Jan 2, 2018

Member

This is not checking if the array is allocated on the stack. You should check for: [stack]

This comment has been minimized.

@lattner

lattner Jan 2, 2018

Collaborator

Yes, exactly, and that is super annoying. Not all targets end up having it be stack allocated in all configurations. I think when the stdlib has checks enabled in particular it doesn't end up getting stack allocated. This is all the class of things I'm interested in improving.

@jckarter

This comment has been minimized.

Member

jckarter commented Jan 2, 2018

Making this a high-level SIL operation makes a lot of sense. cc @milseman, who was experimenting with potential changes to BridgeObject to support the new String implementation. I think this is compatible with his plans.

@milseman

This comment has been minimized.

Contributor

milseman commented Jan 2, 2018

This looks compatible with my use. 👍

@lattner

This comment has been minimized.

Collaborator

lattner commented Jan 2, 2018

Thanks all, and yes, I consider this to be one step forward. Eliminating bridge_object_to_word entirely is a dream :-)

@lattner lattner merged commit 415cd50 into apple:master Jan 2, 2018

4 checks passed

Swift Test Linux Platform 10664 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 53440 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details

@lattner lattner deleted the lattner:classifyBridgeObject2 branch Jan 2, 2018

@jckarter

This comment has been minimized.

Member

jckarter commented Jan 3, 2018

We would like to keep around a way to get the raw payload of a BridgeObject with the trivial bit set. For the case of String in particular we'd use that ability for small string optimization. If we have to change bridge_object_to_word so that it's only defined behavior when the "tagged" bit is set, that'd probably be OK.

@gottesmm

This comment has been minimized.

Member

gottesmm commented Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment