Skip to content

Commit

Permalink
Remove RefElementAddr field from AccessedStorage.
Browse files Browse the repository at this point in the history
- code simplification critical for comprehension
- substantially improves the overhead of AccessedStorage comparison
- as a side effect improves precision of analysis in some cases

AccessedStorage is meant to be an immutable value type that identifies
a storage location with minimal representation. It is used in many global
interprocedural data structures.

The RefElementAddress instruction that it was derived from does not
contribute to the uniqueness of the storage location. It doesn't
belong here. It was being used to create a ProjectionPath, which is an
extremely inneficient way to compare access paths.

Just delete all the code related to that extra field.
  • Loading branch information
atrick committed May 14, 2019
1 parent 8cc013e commit 0d3c614
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 46 deletions.
58 changes: 21 additions & 37 deletions include/swift/SIL/MemAccessUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,43 +29,27 @@ inline bool accessKindMayConflict(SILAccessKind a, SILAccessKind b) {
}

/// Represents the identity of a stored class property as a combination
/// 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
/// of a base and projection.
class ObjectProjection {
SILValue object;
const RefElementAddrInst *REA;
Projection proj;

public:
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; }
ObjectProjection(SILValue object, const Projection &projection)
: object(object), proj(projection) {
assert(object->getType().isObject());
}

SILValue getObject() const { return object; }

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

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

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

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

Expand Down Expand Up @@ -189,8 +173,8 @@ class AccessedStorage {

AccessedStorage(SILValue base, Kind kind);

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

Expand Down Expand Up @@ -295,20 +279,20 @@ class AccessedStorage {
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();
if (!projPath.hasValue() || !otherProjPath.hasValue()) {
if (getKind() != Class || other.getKind() != Class)
// At least one side is an Argument or Yield, or is unidentified.
return false;

// Classes are not uniquely identified by their base. However, if the
// underling objects have identical types and distinct property indices then
// they are distinct storage locations.
auto &proj = getObjectProjection();
auto &otherProj = other.getObjectProjection();
if (proj.getObject()->getType() == otherProj.getObject()->getType()
&& proj.getProjection() != otherProj.getProjection()) {
return true;
}
return projPath.getValue().hasNonEmptySymmetricDifference(
otherProjPath.getValue());
return false;
}

/// Returns the ValueDecl for the underlying storage, if it can be
Expand Down
8 changes: 5 additions & 3 deletions lib/SIL/MemAccessUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,12 @@ AccessedStorage::AccessedStorage(SILValue base, Kind kind) {
case Class: {
// Do a best-effort to find the identity of the object being projected
// from. It is OK to be unsound here (i.e. miss when two ref_element_addrs
// actually refer the same address) because these will be dynamically
// checked.
// actually refer the same address) because these addresses will be
// dynamically checked, and static analysis will be sufficiently
// conservative given that classes are not "uniquely identified".
auto *REA = cast<RefElementAddrInst>(base);
objProj = ObjectProjection(REA);
SILValue Object = stripBorrow(REA->getOperand());
objProj = ObjectProjection(Object, Projection(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 @@ -238,9 +238,9 @@ transformCalleeStorage(const StorageAccessInfo &storage,
if (auto *arg = dyn_cast<SILFunctionArgument>(obj)) {
SILValue argVal = getCallerArg(fullApply, arg->getIndex());
if (argVal) {
auto *instr = storage.getObjectProjection().getInstr();
auto &proj = storage.getObjectProjection().getProjection();
// Remap the argument source value and inherit the old storage info.
return StorageAccessInfo(AccessedStorage(argVal, instr), storage);
return StorageAccessInfo(AccessedStorage(argVal, proj), storage);
}
}
// Otherwise, continue to reference the value in the callee because we don't
Expand Down
7 changes: 3 additions & 4 deletions test/SILOptimizer/access_enforcement_opts.sil
Original file line number Diff line number Diff line change
Expand Up @@ -1352,18 +1352,17 @@ class RefElemClass {
init()
}

// Merge access overlapping scopes. Scope nesting does not need to be
// preserved unless the nested accesses are to identical storage.
// Merge access overlapping scopes.
//
// CHECK-LABEL: sil @ref_elem_c : $@convention(thin) (RefElemClass) -> () {
// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
// CHECK-NEXT: load [[BEGIN]] : $*X
// CHECK-NEXT: end_access [[BEGIN]] : $*X
// CHECK-NEXT: [[REFX:%.*]] = ref_element_addr %0 : $RefElemClass, #RefElemClass.x
// CHECK-NEXT: [[REFY:%.*]] = ref_element_addr %0 : $RefElemClass, #RefElemClass.y
// CHECK-NEXT: [[BEGINX:%.*]] = begin_access [modify] [dynamic] [[REFX]] : $*BitfieldOne
// CHECK: [[BEGINY:%.*]] = begin_access [modify] [dynamic] [[REFY]] : $*Int32
// CHECK: [[BEGINY:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[REFY]] : $*Int32
// CHECK: end_access [[BEGINX]] : $*BitfieldOne
// CHECK-NEXT: end_access [[BEGINY]] : $*Int32
// CHECK-LABEL: } // end sil function 'ref_elem_c'
Expand Down

0 comments on commit 0d3c614

Please sign in to comment.