Skip to content

[Exclusivity] Teach MemAccessUtils about Projection Paths #18892

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 1 commit into from
Aug 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 38 additions & 0 deletions benchmark/single-source/ExistentialPerformance.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import TestsUtils

public let ExistentialPerformance = [
BenchmarkInfo(name: "DistinctClassFieldAccesses", runFunction: run_DistinctClassFieldAccesses, tags: [.unstable, .api, .Array]),
BenchmarkInfo(name: "ExistentialTestArrayConditionalShift_ClassValueBuffer1", runFunction: run_ExistentialTestArrayConditionalShift_ClassValueBuffer1, tags: [.unstable, .api, .Array]),
BenchmarkInfo(name: "ExistentialTestArrayConditionalShift_ClassValueBuffer2", runFunction: run_ExistentialTestArrayConditionalShift_ClassValueBuffer2, tags: [.unstable, .api, .Array]),
BenchmarkInfo(name: "ExistentialTestArrayConditionalShift_ClassValueBuffer3", runFunction: run_ExistentialTestArrayConditionalShift_ClassValueBuffer3, tags: [.unstable, .api, .Array]),
Expand Down Expand Up @@ -114,6 +115,43 @@ public let ExistentialPerformance = [
BenchmarkInfo(name: "ExistentialTestTwoMethodCalls_IntValueBuffer4", runFunction: run_ExistentialTestTwoMethodCalls_IntValueBuffer4, tags: [.unstable]),
]

class ClassWithArrs {
var N: Int = 0
var A: [Int]
var B: [Int]

init(N: Int) {
self.N = N

A = [Int](repeating: 0, count: N)
B = [Int](repeating: 0, count: N)
}

func readArr() {
for i in 0..<self.N {
for j in 0..<i {
let _ = A[j]
let _ = B[j]
}
}
}

func writeArr() {
for i in 0..<self.N {
A[i] = i
B[i] = i
}
}
}

public func run_DistinctClassFieldAccesses(_ N: Int) {
let workload = ClassWithArrs(N: 100)
for _ in 1...N {
workload.writeArr()
workload.readArr()
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the point of this benchmark is to check if the access checks are moved out of the loops. IMO, it's not required to have a benchmark for this, because this can be easily checked with a lit test - as you do.

Copy link
Author

Choose a reason for hiding this comment

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

I discussed the inclusion of this benchmark with @atrick yesterday, was not sure if we should include it or not, decided to add it to the unstable / exclusivity benchmark package - which we do not run by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice to have a separate data point in the public benchmark suite for some part of a larger benchmark that we care about. On the other hand, if there's nothing unique or interesting about the code aside from the exclusivity check, then it doesn't really add value.

protocol Existential {
init()
func doIt() -> Bool
Expand Down
52 changes: 40 additions & 12 deletions include/swift/SIL/MemAccessUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,27 +29,44 @@ inline bool accessKindMayConflict(SILAccessKind a, SILAccessKind b) {
}

/// Represents the identity of a stored class property as a combination
/// of a base and a single projection. Eventually the goal is to make this
/// more precise and consider, casts, etc.
/// of a base, projection and projection path.
/// we pre-compute the base and projection path, even though we can
/// lazily do so, because it is more expensive otherwise
/// We lazily compute the projection path,
/// In the rare occasions we need it, because of Its destructor and
/// its non-trivial copy constructor
class ObjectProjection {
SILValue object;
const RefElementAddrInst *REA;
Projection proj;

public:
ObjectProjection(SILValue object, const Projection &proj)
: object(object), proj(proj) {
assert(object->getType().isObject());
}
ObjectProjection(const RefElementAddrInst *REA)
: object(stripBorrow(REA->getOperand())), REA(REA),
proj(Projection(REA)) {}

ObjectProjection(SILValue object, const RefElementAddrInst *REA)
: object(object), REA(REA), proj(Projection(REA)) {}

const RefElementAddrInst *getInstr() const { return REA; }

SILValue getObject() const { return object; }

const Projection &getProjection() const { return proj; }

const ProjectionPath getProjectionPath() const {
return ProjectionPath::getProjectionPath(stripBorrow(REA->getOperand()),
REA)
.getValue();
}

bool operator==(const ObjectProjection &other) const {
return object == other.object && proj == other.proj;
return getObject() == other.getObject() &&
getProjection() == other.getProjection();
}

bool operator!=(const ObjectProjection &other) const {
return object != other.object || proj != other.proj;
return !operator==(other);
}
};

Expand Down Expand Up @@ -168,8 +185,8 @@ class AccessedStorage {

AccessedStorage(SILValue base, Kind kind);

AccessedStorage(SILValue object, Projection projection)
: objProj(object, projection) {
AccessedStorage(SILValue object, const RefElementAddrInst *REA)
: objProj(object, REA) {
initKind(Class);
}

Expand Down Expand Up @@ -255,8 +272,19 @@ class AccessedStorage {
}

bool isDistinctFrom(const AccessedStorage &other) const {
return isUniquelyIdentified() && other.isUniquelyIdentified()
&& !hasIdenticalBase(other);
if (isUniquelyIdentified() && other.isUniquelyIdentified()) {
return !hasIdenticalBase(other);
}
if (getKind() != Class || other.getKind() != Class) {
return false;
}
if (getObjectProjection().getProjection() ==
other.getObjectProjection().getProjection()) {
return false;
}
auto projPath = getObjectProjection().getProjectionPath();
auto otherProjPath = other.getObjectProjection().getProjectionPath();
return projPath.hasNonEmptySymmetricDifference(otherProjPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you just use AliasAnalysis::isNoAlias for this check?
Internally it performs the same check with projection paths as you do here + several other checks. So it is more accurate.

Copy link
Author

Choose a reason for hiding this comment

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

we certainly could. it is higher overhead of course: we know for sure we have two address projections and just need to check their projection paths, which is part of isNoAlias as you say. I looked for other uses of both in the optimizer, for consistency, from what I saw, when we have a similar situation ( LoadStoreOpt and ARC ) we called hasNonEmptySymmetricDifference directly. but I can change that.

Copy link
Author

Choose a reason for hiding this comment

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

i.e. the extra checks / code paths are known to be false + we are fetching AliasAnalysis / tying it to memory access utilities, unlike load store utilities which avoided that, but it should work

Copy link
Author

Choose a reason for hiding this comment

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

I just added Alais Analysis on my machine: we don't have access to a pass manager in accessed storage, which means we have to create AA outside and pass it in to the class' constructor, polluting the entire code and call sites to accessed storage for no change in behavior, it also looks ugly by forcing an analysis from the SIL Optimizer library into a SIL library utility

Copy link
Contributor

Choose a reason for hiding this comment

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

A pass could use AliasAnalysis, and we could teach AliasAnalysis about AccessedStorage. As it is, the MemAccessUtils check will be missing TBAA and escape analysis. I would want a different AliasAnalysis entry point for AccessedStorage though. Almost all of the existing AliasAnlysis code is irrelevant, so it would be very inefficient and hard to reason about what really happens in the formal access case.

}

/// Returns the ValueDecl for the underlying storage, if it can be
Expand Down
3 changes: 1 addition & 2 deletions lib/SIL/MemAccessUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,7 @@ AccessedStorage::AccessedStorage(SILValue base, Kind kind) {
// actually refer the same address) because these will be dynamically
// checked.
auto *REA = cast<RefElementAddrInst>(base);
SILValue Object = stripBorrow(REA->getOperand());
objProj = ObjectProjection(Object, Projection(REA));
objProj = ObjectProjection(REA);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions lib/SILOptimizer/Analysis/AccessedStorageAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,9 @@ transformCalleeStorage(const StorageAccessInfo &storage,
if (auto *arg = dyn_cast<SILFunctionArgument>(obj)) {
SILValue argVal = getCallerArg(fullApply, arg->getIndex());
if (argVal) {
auto &proj = storage.getObjectProjection().getProjection();
auto *instr = storage.getObjectProjection().getInstr();
// Remap the argument source value and inherit the old storage info.
return StorageAccessInfo(AccessedStorage(argVal, proj), storage);
return StorageAccessInfo(AccessedStorage(argVal, instr), storage);
}
}
// Otherwise, continue to reference the value in the callee because we don't
Expand Down
37 changes: 37 additions & 0 deletions test/SILOptimizer/licm_exclusivity.swift
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// RUN: %target-swift-frontend -O -enforce-exclusivity=checked -emit-sil -Xllvm -debug-only=sil-licm -primary-file %s 2>&1 | %FileCheck %s --check-prefix=TEST1
// RUN: %target-swift-frontend -O -enforce-exclusivity=checked -emit-sil -Xllvm -debug-only=sil-licm -primary-file %s 2>&1 | %FileCheck %s --check-prefix=TEST2
// RUN: %target-swift-frontend -O -enforce-exclusivity=checked -emit-sil -primary-file %s | %FileCheck %s --check-prefix=TESTSIL
// RUN: %target-swift-frontend -O -enforce-exclusivity=checked -emit-sil -Xllvm -debug-only=sil-licm -whole-module-optimization %s 2>&1 | %FileCheck %s --check-prefix=TEST3
// RUN: %target-swift-frontend -O -enforce-exclusivity=checked -emit-sil -whole-module-optimization %s | %FileCheck %s --check-prefix=TESTSIL2

// REQUIRES: optimized_stdlib,asserts

// TEST1-LABEL: Processing loops in {{.*}}run_ReversedArray{{.*}}
Expand Down Expand Up @@ -44,3 +47,37 @@ public func count_unicodeScalars(_ s: String.UnicodeScalarView) {
count += 1
}
}


public class ClassWithArrs {
var N: Int = 0
var A: [Int]
var B: [Int]

init(N: Int) {
self.N = N

A = [Int](repeating: 0, count: N)
B = [Int](repeating: 0, count: N)
}

// TEST3-LABEL: Processing loops in {{.*}}ClassWithArrsC7readArr{{.*}}
// TEST3: Hoist and Sink pairs attempt
// TEST3: Hoisted
// TEST3: Successfully hosited and sank pair
// TEST3: Hoisted
// TEST3: Successfully hosited and sank pair
// TESTSIL2-LABEL: sil @$S16licm_exclusivity13ClassWithArrsC7readArryyF : $@convention(method) (@guaranteed ClassWithArrs) -> () {
// TESTSIL2: [[R1:%.*]] = ref_element_addr %0 : $ClassWithArrs, #ClassWithArrs.A
// TESTSIL2: [[R2:%.*]] = ref_element_addr %0 : $ClassWithArrs, #ClassWithArrs.B
// TESTSIL2: begin_access [read] [static] [no_nested_conflict] [[R1]]
// TESTSIL2: begin_access [read] [static] [no_nested_conflict] [[R2]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that this really checks if the begin_accesses are moved out of the loop?
There could be basic block headers between the checked lines

Copy link
Author

Choose a reason for hiding this comment

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

Yes: see TEST3 above these lines: it is the debug output of LICM - saying that we hoisted the begin_accesses auto of the loop.

Copy link
Author

Choose a reason for hiding this comment

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

And the checks, after the new support, turned from dynamic to static.

public func readArr() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you also test the writeArr function?

Copy link
Author

Choose a reason for hiding this comment

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

The accesses to 'A' and 'B' in writeArr are begin_unpaired_access which we cannot (currently) hoist or merge (for safety) - if we end up supporting those in the future (which, from what I saw so far, is not high on our list and is too risky) then we can add such a lit test of course

Copy link
Author

Choose a reason for hiding this comment

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

It is also why I did not add such a write function to lit test, but it is part of the benchmark.

for i in 0..<self.N {
for j in 0..<i {
let _ = A[j]
let _ = B[j]
}
}
}
}