[sil-optimizer] Make InstructionDeleter and related APIs to use an InstModCallback instead of a notification callback. #37016
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci test |
gottesmm
commented
Apr 22, 2021
| /// default operation inline. What is nice about this from a perf perspective is | ||
| /// that in a loop this property should predict well since you have a single | ||
| /// branch that is going to go the same way everytime. | ||
| class InstModCallbacks { |
There was a problem hiding this comment.
I just moved this to work around forward declaration issues.
|
Build failed |
|
Build failed |
…stModCallback instead of a notification callback. I recently have been running into the issue that many of these APIs perform the deletion operation themselves and notify the caller it is going to delete instead of allowing the caller to specify how the instruction is deleted. This causes interesting semantic issues (see the loop in deleteInstruction I simplified) and breaks composition since many parts of the optimizer use InstModCallbacks for this purpose. To fix this, I added a notify will be deleted construct to InstModCallback. In a similar way to the rest of it, if the notify is not set, we do not call any code implying that we should have good predictable performance in loops since we will always skip the function call. I also changed InstModCallback::deleteInst() to notify before deleting so we have a default safe behavior. All previous use sites of this API do not care about being notified and the only new use sites of this API are in InstructionDeleter that perform special notification behavior (it notifies for certain sets of instructions it is going to delete before it deletes any of them). To work around this, I added a bool to deleteInst to control this behavior and defaulted to notifying. This should ensure that all other use sites still compose correctly.
gottesmm
force-pushed
the
pr-eb1b4b484d8990f6f5cb6706dba952b3e27cdc59
branch
from
35bb49c to
7b55cbc
Compare
gottesmm
changed the title
|
@swift-ci test |
atrick
approved these changes
Apr 27, 2021
|
Thanks will do. |
|
@gottesmm And I almost forgot. We shouldn't need two copies of |
|
@atrick I will fix that as well. |
|
@atrick the callback in eraseFromParentWithDebugInsts is for a notification, not the delete. That is why I did both. |
gottesmm
added a commit
that referenced
this pull request
Apr 27, 2021
…481e36205ea258 [sil-optimizer] Fix feedback from #37016 and finish removing confusing constructors
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.