-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
}; | ||
|
||
|
@@ -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); | ||
} | ||
|
||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't you just use AliasAnalysis::isNoAlias for this check? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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{{.*}} | ||
|
@@ -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]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes: see There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't you also test the writeArr function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The accesses to 'A' and 'B' in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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] | ||
} | ||
} | ||
} | ||
} |
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 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.
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 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.
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.
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.