[WIP] SemanticARCOpts: Don't copy let properties of classes. #26810
Conversation
jckarter
changed the title
|
@swift-ci Please benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
@swift-ci Please test |
|
@swift-ci Please benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
@swift-ci Please benchmark |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
Interesting. We are clearly with the global thing hitting more retain/release (look at those code-size improvements!) Wonder what the perf change is from. |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
@gottesmm I would guess that global string constants could account for a lot of the code size difference, since even though they have no-op refcounts, the compiler still probably naively generates retains and releases for them. |
| // CHECK-NEXT: apply | ||
| // CHECK-NEXT: destroy_value | ||
| sil [ossa] @do_copy_let_properties_with_borrowed_base_that_does_not_dominate : $@convention(thin) (@owned ClassLet) -> () { | ||
| bb0(%x : $ClassLet): |
There was a problem hiding this comment.
I am surprised that this is parsing in ossa. Shouldn't this have @owned in front of it? I wouldn't worry about this (I will look into it later...) but can you put in the @owned, @guaranteed on arguments?
| bb0(%x : $ClassLet): | ||
| %f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> () | ||
|
|
||
| %p = ref_element_addr %x : $ClassLet, #ClassLet.aLetTuple |
There was a problem hiding this comment.
Maybe strip off casts as well so you can dig for the argument?
| auto baseObject = storage.getObject(); | ||
| // A guaranteed argument trivially keeps the base alive for the duration of | ||
| // the projection. | ||
| if (auto arg = dyn_cast<SILFunctionArgument>(baseObject)) { |
There was a problem hiding this comment.
why not try to strip off casts to see if you can find a guaranteed argument? I.e. can you handle a case where you have some sort of cast (perhaps an unchecked down cast or upcast) from a guaranteed parameter? If you are busy/no worries, but I think it is an important case missing from this. I am happy if it comes in via a future commit.
|
|
||
| // See if there's a borrow of the base object our load is based on. | ||
| SILValue borrowInst; | ||
| if (isa<BeginBorrowInst>(baseObject) |
There was a problem hiding this comment.
This should be extracted out into a helper function or should be constructed in a closure.
There was a problem hiding this comment.
Not until it's used somewhere else, IMO. Inline code can't have multiple dependencies.
|
|
||
| BeginBorrowInst *singleBorrow = nullptr; | ||
| for (auto use : baseObject->getUses()) { | ||
| if (auto borrow = dyn_cast<BeginBorrowInst>(use->getUser())) { |
There was a problem hiding this comment.
Can you use * on all auto pointers?
|
|
||
| // Use the linear lifetime checker to check whether the copied | ||
| // value is dominated by the lifetime of the borrow it's based on. | ||
| if (borrowInst) { |
There was a problem hiding this comment.
Can you invert the if condition here and early exit if borrowInst was not found? It reduces indentation.
| } | ||
|
|
||
| SmallPtrSet<SILBasicBlock *, 4> visitedBlocks; | ||
| DeadEndBlocks deadEndBlocks(&f); |
There was a problem hiding this comment.
DeadEndBlocks IIRC is relatively expensive. I would hoist it all the way out. I think generally we want one per function pass invocation.
If the lifetime of a value copied out of a let property in a class is dominated by the lifetime of the base object it was copied from, then we don't need to copy at all, since the value of the `let` is guaranteed as long as the base object is. `let` globals are in turn always guaranteed. rdar://problem/54531607
jckarter
force-pushed
the
class-let-property-borrow
branch
from
1744dbe to
7ca6587
Compare
|
@swift-ci Please benchmark |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci Please benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -OnoneCode size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
If the lifetime of a value copied out of a let property in a class is dominated by the lifetime
of the base object it was copied from, then we don't need to copy at all, since the value of the
letis guaranteed as long as the base object is. rdar://problem/54531607