Skip to content

Comment on why we can't replace copy_addr instructions with moves in arc-inert blocks #5138

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 11, 2016
Merged

Comment on why we can't replace copy_addr instructions with moves in arc-inert blocks #5138

merged 1 commit into from
Oct 11, 2016

Conversation

shajrawi
Copy link

@shajrawi shajrawi commented Oct 5, 2016 •
edited
Loading

radar rdar://problem/24011383

This concludes this radar by adding a comment on why copy_addr instructions can't be turned into moves in simplifyProgramTerminationBlock()

@shajrawi
Copy link
Author

shajrawi commented Oct 5, 2016

@gottesmm Can you please review?

@shajrawi
Copy link
Author

shajrawi commented Oct 5, 2016

@swift-ci Please smoke test

@jckarter
Copy link
Contributor

jckarter commented Oct 5, 2016

Why does it have to be ARC-inert? It should always be possible to break a copy_addr [init] [take] into a load and store.

@gottesmm
Copy link
Contributor

gottesmm commented Oct 5, 2016

@jckarter That is not what is happening here. What is happening here is that we are changing copy_addr in blocks that we know are leaking to just be moves.

@gottesmm gottesmm self-assigned this Oct 6, 2016
@jckarter
Copy link
Contributor

jckarter commented Oct 6, 2016

The transformation here is always valid; copy_addr [init] [take] should always be equivalent to load; store for a loadable type. I was wondering why we were constraining the transformation only to 'ARC-inert' blocks; AFAICS this transformation should always be OK.

@gottesmm
Copy link
Contributor

gottesmm commented Oct 9, 2016

@shajrawi Sorry for the delay. I am reviewing this now.

gottesmm
gottesmm requested changes Oct 9, 2016
View reviewed changes
Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

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

I think we can make this even more aggressive. Given that we are always going to leak in these blocks, we can also ignore:

copy_addr [take] %0 to %1

lib/SILOptimizer/Transforms/SimplifyCFG.cpp
!isa<ReleaseValueInst>(I) && !isa<DestroyAddrInst>(I))
if (!isa<StrongReleaseInst>(I) && !isa<UnownedReleaseInst>(I) &&
!isa<ReleaseValueInst>(I) && !isa<DestroyAddrInst>(I) &&
!isa<CopyAddrInst>(I))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe refactor this query into a helper function?

lib/SILOptimizer/Transforms/SimplifyCFG.cpp
}

// Add moves for every copy_addr
for (auto I : InstsToReplace) {
Copy link
Contributor

Choose a reason for hiding this comment

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

May be good to make it clear that this is a pointer by making it an auto *

lib/SILOptimizer/Transforms/SimplifyCFG.cpp
// Add moves for every copy_addr
for (auto I : InstsToReplace) {
SILBuilderWithScope Builder(I);
auto CAI = cast<CopyAddrInst>(I);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also make this an auto *.

lib/SILOptimizer/Transforms/SimplifyCFG.cpp
auto CAI = cast<CopyAddrInst>(I);
assert(CAI && "Expected CopyAddrInst in InstsToReplace set");
auto Loc = CAI->getLoc();
auto LI = Builder.createLoad(Loc, CAI->getSrc());
Copy link
Contributor

@gottesmm gottesmm Oct 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Also this please make an auto * ; ).

@gottesmm
Copy link
Contributor

gottesmm commented Oct 9, 2016

The reason why we do not want to eliminate retains is we want to make sure that we fail at the actual program termination point, not before.

There is a possibility where we could by removing the retain change a balanced retain count to be unbalanced resulting in us touching a deallocated object.

In contrast, removing a release just makes us leak even more, which we are fine with.

@jckarter
Copy link
Contributor

Changing copy_addr [take] %0 to %1 into a take-initialization is not safe in all cases. If the type being operated on contains weak references or other side references, it will corrupt the weak reference table if you fail to deregister the value currently at %1. Maybe it's safe for all currently loadable types, but if we move in the direction of semantic ARC and start representing all types as SSA values, would this be a liability?

Again, copy_addr [take] [init] are load; store are always equivalent, so I don't understand why we're limiting this transformation to unreachable blocks.

@shajrawi
Copy link
Author

@gottesmm can you please answer @jckarter 's input, I feel you are more qualified to answer his concerns.
I also updated the PR with all the changes you requested.

@shajrawi
Copy link
Author

@swift-ci Please smoke test

@shajrawi
Copy link
Author

Linux smoke test failed for no fault of this PR:
/home/buildnode/disk2/workspace/swift-PR-Linux-smoke-test@2/branch-master/swiftpm/Tests/XcodeprojTests/GenerateXcodeprojTests.swift:58:21: error: use of unresolved identifier 'ProjectGenerationError'
} catch ProjectGenerationError.xcconfigOverrideNotFound(let path) {

will re-run

@shajrawi
Copy link
Author

@swift-ci Please smoke test

@gottesmm
Copy link
Contributor

Joe, can we talk about this tomorrow in person?

@gottesmm
Copy link
Contributor

@jckarter I mean ; )

@shajrawi shajrawi changed the title Replace take-initialization copy_addr instructions with moves in arc-inert blocks Comment on why we can't replace copy_addr instructions with moves in arc-inert blocks Oct 11, 2016
@shajrawi
Copy link
Author

Updated the PR: removed code modifications and replaced it with a comment that sums-up the offline discussion with @jckarter and @gottesmm

@shajrawi
Copy link
Author

@swift-ci Please smoke test

@shajrawi shajrawi merged commit b05d3c2 into swiftlang:master Oct 11, 2016
@shajrawi shajrawi deleted the copy_addr_to_moves branch October 11, 2016 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gottesmm gottesmm gottesmm approved these changes

Assignees

@gottesmm gottesmm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shajrawi @jckarter @gottesmm