Skip to content

libswift: implement ReleaseDevirtualizer in Swift #40800

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

Merged
merged 13 commits into from
Jan 20, 2022

Conversation

MaxDesiatov
Copy link
Contributor

Implements one more SILOptimizer pass in Swift.

@MaxDesiatov MaxDesiatov requested a review from eeckstein January 11, 2022 19:05
@MaxDesiatov MaxDesiatov force-pushed the maxd/release-devirtualizer branch from 199d75c to de610ab Compare January 11, 2022 19:08
@gottesmm
Copy link
Contributor

Can you be sure whenever you are porting functions to also add an additional test line to the test that tests the pass. I think we still should test both the legacy/swift pass for now. For instance, you could add an extra test line to this file test/SILOptimizer/devirt_release.sil that way we make sure both passes are correct.

@gottesmm
Copy link
Contributor

Also, thank you for working on these! I think it is important that we start experimenting with writing swift in swift to find out what idioms work/need to be changed or stay the same.

@MaxDesiatov
Copy link
Contributor Author

Can you be sure whenever you are porting functions to also add an additional test line to the test that tests the pass. I think we still should test both the legacy/swift pass for now. For instance, you could add an extra test line to this file test/SILOptimizer/devirt_release.sil that way we make sure both passes are correct.

What extra line should be added there? Is there a sil-opt flag that allows switching between legacy and new Swift passes?

@MaxDesiatov MaxDesiatov changed the title libswift: implement ReleaseDevirtualizer in Swift libswift: implement ReleaseDevirtualizer in Swift Jan 11, 2022
@MaxDesiatov
Copy link
Contributor Author

I think we still should test both the legacy/swift pass for now.

Also, this will make Windows tests fail, since Swift passes aren't built for Windows yet.

@MaxDesiatov MaxDesiatov force-pushed the maxd/release-devirtualizer branch 2 times, most recently from bab1560 to a8a2dd4 Compare January 11, 2022 19:21
@MaxDesiatov
Copy link
Contributor Author

@eeckstein the Swift bridge to RCIdentityAnalysis is going to be removed as soon as I'm sure my use-def walk is good enough, but I wanted to get some preliminary feedback on the overall approach. Thanks!

@MaxDesiatov MaxDesiatov force-pushed the maxd/release-devirtualizer branch from a8a2dd4 to 3591ae4 Compare January 12, 2022 10:49
Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!
See my comments inline.

@@ -9,5 +9,6 @@
if(SWIFT_ENABLE_EXPERIMENTAL_STRING_PROCESSING)
add_subdirectory(ExperimentalRegex)
endif()
add_subdirectory(AST)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put SubstitutionMap directly into SIL.
It's like SILType, which is a wrapper around the AST Type. Except that in C++ we don't have a SIL wrapper around SubstitutionMap, but that doesn't matter for the swift SubstitutionMap.

# Licensed under Apache License v2.0 with Runtime Library Exception
#
# See http://swift.org/LICENSE.txt for license information
# See http://swift.org/CONTRIBUTORS.txt for Swift project authors

swift_compiler_sources(Optimizer
AliasAnalysis.swift
CalleeAnalysis.swift)
CalleeAnalysis.swift
RCIdentityAnalysis.swift)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you said, please remove that when you have your own use-def walk.
Please put your own utility function for that directly into ReleaseDevirtualizer.swift.
As we discussed offline, in future we can use the new EscapeInfo utility to do a better analysis.

_ functionInfo: RCIdentityFunctionInfo
) {
let allocRefInstruction = deallocStackRef.allocRef
let rcRoot = functionInfo.getRCIdentityRoot(release.operands[0].value)
Copy link
Contributor

@eeckstein eeckstein Jan 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename rcRoot to something like rootObject


precondition(rcRoot === root)

guard rcRoot === allocRefInstruction else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for a guard. You can use an if.
Also, no need for ===. Value has a == operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allocRefInstruction here is of AllocRefInst type, while root is of Value. For some reason == does not automatically cast AllocRefInst to Value, would you prefer me to cast explicitly, or keep === as it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, Value isn't Equatable right now, should the former inherit from the latter? Otherwise != isn't currently defined on Value arguments to convert this from guard to if in a nice way.

return
}

guard rcRoot === release.operands[0].value else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is just some testing code and you'll remove it, right?

@@ -84,6 +89,14 @@ struct PassContext {

RefCountingInst_setIsAtomic(instruction.bridged, isAtomic)
}

func getDealloc(for type: Type) -> Function? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename this function to getDestructor(ofClass: Type).
We use the term "Destructor" also in the AST, e.g. DestructorDecl.
And as we don't have separate swift types for the different AST types (we just pack everything into Type), it should be clear from the label that the argument should be a class type.

function: Value,
_ substitutionMap: SubstitutionMap,
arguments: [Value]
) -> FunctionRefInst {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FunctionRefInst -> ApplyInst

substitutionMap.bridged, valuesRef
)
}
return functionRef.getAs(FunctionRefInst.self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FunctionRefInst -> ApplyInst

arguments: [Value]
) -> FunctionRefInst {
notifyInstructionsChanged()
let functionRef = arguments.withBridgedValues { valuesRef in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

functionRef -> apply

@eeckstein
Copy link
Contributor

I think we still should test both the legacy/swift pass for now.

@gottesmm Legacy passes are tested with the Windows PR tests. So no need to test them on macOS/Linux (they don't run on these platforms anyway).

@gottesmm
Copy link
Contributor

@eeckstein ok. My fear is that will be forgotten. Perhaps we add notes into each of both passes that explain this? Otherwise I am scared that people will forget (I probably will with enough time). Maybe write that in a comment in the individual test itself? That would be good enough.

@MaxDesiatov MaxDesiatov force-pushed the maxd/release-devirtualizer branch 5 times, most recently from d8a0143 to 15ee311 Compare January 17, 2022 16:10
@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@MaxDesiatov MaxDesiatov marked this pull request as ready for review January 17, 2022 16:11
@MaxDesiatov
Copy link
Contributor Author

@swift-ci Please smoke benchmark

return AvailabilityContext::forDeploymentTarget(ctxt).
isContainedIn(ctxt.getSwift51Availability());
return AvailabilityContext::forDeploymentTarget(ctxt).isContainedIn(
ctxt.getSwift51Availability());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was applied by the formatter.

auto *NTD = AllocType.getASTType()->getAnyNominal();
auto AllocSubMap = AllocType.getASTType()
->getContextSubstitutionMap(M.getSwiftModule(), NTD);

DeallocType = DeallocType->substGenericArgs(M, AllocSubMap, context);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed dead code here

@MaxDesiatov MaxDesiatov force-pushed the maxd/release-devirtualizer branch from 15ee311 to 8f80374 Compare January 17, 2022 16:15
@MaxDesiatov MaxDesiatov requested a review from eeckstein January 17, 2022 16:15
@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Jan 17, 2022

@eeckstein I think all feedback requests were addressed, I've verified that use-def walk behaves in the same way as it does in the C++ version. RCIdentityAnalysis use was cleaned up in the Swift version and it's no longer bridged.

@gottesmm I've added a comment to the test that clarifies how the legacy and the new Swift pass are tested on Windows and other platforms.

@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
ArrayAppendOptionals 1330 2480 +86.5% 0.54x (?)
 
Improvement OLD NEW DELTA RATIO
ObjectiveCBridgeStubFromNSDateRef 5030 4200 -16.5% 1.20x (?)

Code size: -O

Performance (x86_64): -Osize

Improvement OLD NEW DELTA RATIO
ArrayAppendOptionals 2470 2020 -18.2% 1.22x (?)
FlattenListFlatMap 6820 6003 -12.0% 1.14x (?)

Code size: -Osize

Performance (x86_64): -Onone

Improvement OLD NEW DELTA RATIO
StringBuilderLong 2060 1900 -7.8% 1.08x (?)

Code size: -swiftlibs

How to read the data The 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
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@MaxDesiatov MaxDesiatov requested a review from eeckstein January 19, 2022 14:21
Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!
Still one small comment.


let opType = operand.type

return opType.tupleElements.filter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately filter is pretty bad for performance because it creates a new Array (involving memory allocation, etc.).
You can do lazy.filter, but then you cannot use count.
Or you do a good old fashioned for ... in ... loop (which would have the benefit that you abort as soon as the count exceeds one.


let structType = operand.type

return structType.getStructFields(in: function).filter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same here

@MaxDesiatov MaxDesiatov force-pushed the maxd/release-devirtualizer branch from da78773 to 6a3d2a0 Compare January 19, 2022 15:36
@MaxDesiatov MaxDesiatov requested a review from eeckstein January 19, 2022 15:36
@MaxDesiatov MaxDesiatov force-pushed the maxd/release-devirtualizer branch 2 times, most recently from 06691fb to cebeea5 Compare January 19, 2022 15:59
@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

MaxDesiatov and others added 13 commits January 19, 2022 18:51
* `Function.argumentTypes` and `Function.resultType`
* `Type.isNominal`, `Type.isClass`, `Type.isTuple`, `Type.isStruct` and `Type.isEnum`
* `Type.getFieldIndexOfNominal`
* `Type.getFieldTypeOfNominal`
* `Type.tupleElements`
* `Type.description` for better debugging

# Conflicts:
#	SwiftCompilerSources/Sources/SIL/Type.swift
#	SwiftCompilerSources/Sources/SIL/Utils.swift
#	SwiftCompilerSources/Sources/SIL/Value.swift
#	include/swift/SIL/SILBridging.h
#	lib/SIL/Utils/SILBridging.cpp
@MaxDesiatov MaxDesiatov force-pushed the maxd/release-devirtualizer branch from cebeea5 to 1f53563 Compare January 19, 2022 18:52
@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 1f53563

@MaxDesiatov
Copy link
Contributor Author

C++ interop std_vector test failure is unrelated

@eeckstein eeckstein merged commit 6d4f2d9 into main Jan 20, 2022
@eeckstein eeckstein deleted the maxd/release-devirtualizer branch January 20, 2022 07:17
@MaxDesiatov
Copy link
Contributor Author

@eeckstein thanks for your detailed review feedback and guidance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants