-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
Conversation
199d75c
to
de610ab
Compare
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. |
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. |
What extra line should be added there? Is there a |
ReleaseDevirtualizer
in Swift
Also, this will make Windows tests fail, since Swift passes aren't built for Windows yet. |
bab1560
to
a8a2dd4
Compare
@eeckstein the Swift bridge to |
a8a2dd4
to
3591ae4
Compare
There was a problem hiding this 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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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? { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functionRef
-> apply
@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). |
@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. |
d8a0143
to
15ee311
Compare
@swift-ci please smoke test |
@swift-ci Please smoke benchmark |
return AvailabilityContext::forDeploymentTarget(ctxt). | ||
isContainedIn(ctxt.getSwift51Availability()); | ||
return AvailabilityContext::forDeploymentTarget(ctxt).isContainedIn( | ||
ctxt.getSwift51Availability()); |
There was a problem hiding this comment.
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); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed dead code here
15ee311
to
8f80374
Compare
@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. @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. |
@swift-ci please smoke test |
@swift-ci please benchmark |
Performance (x86_64): -O
Code size: -OPerformance (x86_64): -Osize
Code size: -OsizePerformance (x86_64): -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 smoke test |
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same here
da78773
to
6a3d2a0
Compare
06691fb
to
cebeea5
Compare
@swift-ci please smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks!
* `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
cebeea5
to
1f53563
Compare
@swift-ci please smoke test |
@swift-ci please test |
Build failed |
C++ interop std_vector test failure is unrelated |
@eeckstein thanks for your detailed review feedback and guidance! |
Implements one more SILOptimizer pass in Swift.