Skip to content

Teach stripCasts (and getUnderlyingObject) about begin_access. #32842

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

Merged
merged 1 commit into from
Jul 13, 2020
Merged

Teach stripCasts (and getUnderlyingObject) about begin_access. #32842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions include/swift/SIL/MemAccessUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@

namespace swift {

/// Get the base address of a formal access by stripping access markers and
/// borrows.
/// Get the base address of a formal access by stripping access markers.
///
/// If \p v is an address, then the returned value is also an address
/// (pointer-to-address is not stripped).
Expand Down
10 changes: 6 additions & 4 deletions lib/SIL/Utils/InstructionUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,10 @@ SILValue swift::stripCastsWithoutMarkDependence(SILValue V) {
V = stripSinglePredecessorArgs(V);

auto K = V->getKind();
if (isRCIdentityPreservingCast(K) ||
K == ValueKind::UncheckedTrivialBitCastInst ||
K == ValueKind::EndCOWMutationInst) {
if (isRCIdentityPreservingCast(K)
|| K == ValueKind::UncheckedTrivialBitCastInst
|| K == ValueKind::BeginAccessInst
|| K == ValueKind::EndCOWMutationInst) {
V = cast<SingleValueInstruction>(V)->getOperand(0);
continue;
}
Expand All @@ -145,7 +146,8 @@ SILValue swift::stripCasts(SILValue v) {
auto k = v->getKind();
if (isRCIdentityPreservingCast(k)
|| k == ValueKind::UncheckedTrivialBitCastInst
|| k == ValueKind::MarkDependenceInst) {
|| k == ValueKind::MarkDependenceInst
|| k == ValueKind::BeginAccessInst) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eeckstein I don't know why stripCasts does not check EndCOWMutation

Copy link
Contributor

@eeckstein eeckstein Jul 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's a longer story: when I added end_cow_mutation, I came to the conclusion that those global stripWhatEver utility functions are not such a good idea.
Let's take stripCasts as example: every optimization has (or could have) a different view of what a cast instruction is. For EscapeAnalysis a begin_access is the same as a cast. But that's not necessarily true for other optimizations, which might need to treat begin_access separately.

I think it's better to let each optimization/analysis contain there own strip... functions, even if this means some code duplication.
Then it's much easier to modify them and add instructions as needed without worrying about other optimizations.

That's why I added end_cow_mutation only at those places where it was really needed and not in the global strip functions.
... and unfortunately, never came back to remove the global strip functions at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The strip utilities are not good abstractions for a high-level IR like SIL. I also did not want to modify them when adding the access markers. But not handling access markers has been blocking progress recently, and migrating passes away from the strip functions will take more time.

We now have an AccessUseDefVisitor that more properly abstracts over address and pointer operations, allowing passes to override some of the behavior without copying a giant switch statement everywhere. I'll demonstrate this in some upcoming PRs... And for every use-def abstraction, we need a matching def-use abstraction.

More importantly, an abstraction is useful if we can use it impose some order on SIL, defining which patterns are valid SIL and enforcing that in the verifier. In some situations, we can't simply bail-out on unrecognized patterns, so we need an abstraction that's guaranteed to work everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't get me wrong: I think this PR is fine. I just wanted to answer the question why end_cow_mutation is not handled in stripCasts.

v = cast<SingleValueInstruction>(v)->getOperand(0);
continue;
}
Expand Down
9 changes: 7 additions & 2 deletions lib/SIL/Utils/Projection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,8 +371,13 @@ Optional<ProjectionPath> ProjectionPath::getProjectionPath(SILValue Start,

auto Iter = End;
while (Start != Iter) {
// end_cow_mutation is not a projection, but we want to "see through" it.
if (!isa<EndCOWMutationInst>(Iter)) {
// end_cow_mutation and begin_access are not projections, but we need to be
// able to form valid ProjectionPaths across them, otherwise optimization
// passes like RLE/DSE cannot recognize their locations.
//
// TODO: migrate users to getProjectionPath to the AccessPath utility to
// avoid this hack.
if (!isa<EndCOWMutationInst>(Iter) && !isa<BeginAccessInst>(Iter)) {
Projection AP(Iter);
if (!AP.isValid())
break;
Expand Down
2 changes: 1 addition & 1 deletion test/SILOptimizer/escape_analysis.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1938,7 +1938,7 @@ bb0(%0 : $*SomeData):
// CHECK: to set_deallocating %0 : $IntWrapper
// CHECK: NoEscape: %3 = ref_element_addr %0 : $IntWrapper, #IntWrapper.property
// CHECK: to set_deallocating %0 : $IntWrapper
// CHECK: MayEscape: %4 = begin_access [modify] [dynamic] [no_nested_conflict] %3 : $*Int64
// CHECK: NoEscape: %4 = begin_access [modify] [dynamic] [no_nested_conflict] %3 : $*Int64
// CHECK: to set_deallocating %0 : $IntWrapper
sil @testEndAccess : $@convention(thin) () -> () {
bb0:
Expand Down