Add an AccessPath abstraction and formalize memory access #34126
Conversation
|
Discussion in #33121 |
|
@swift-ci test |
|
@swift-ci test source compatibility |
|
Build failed |
|
Build failed |
atrick
force-pushed
the
add-accesspath
branch
from
0fd6be8 to
810a620
Compare
|
@swift-ci test |
|
Build failed |
|
Build failed |
|
@swift-ci test |
|
Build failed |
| case AccessedStorage::Unidentified: | ||
| return getValue(); // Can be invalid for Unidentified storage. | ||
| case AccessedStorage::Global: | ||
| return SILValue(); |
There was a problem hiding this comment.
Why is a global not a valid root?
| @@ -427,18 +464,17 @@ class AccessedStorage { | |||
|
|
|||
| /// If this is a uniquely identified formal access, then it cannot | |||
| /// alias with any other uniquely identified access to different storage. | |||
| /// | |||
| /// This determines whether access markers may conflict, so it cannot assume | |||
| /// that exclusivity is enforced. | |||
| bool isUniquelyIdentified() const { | |||
| switch (getKind()) { | |||
| case Box: | |||
| case Stack: | |||
| case Global: | |||
| return true; | |||
There was a problem hiding this comment.
And here: why is a global uniquely identified, while a class is not?
|
|
||
| // Special-case this indirect enum pattern: | ||
| // unchecked_take_enum_data_addr -> load -> project_box | ||
| // (the individual load and project_box are not access projections) |
There was a problem hiding this comment.
Are you sure you want to model indirect enum cases as address projections?
It might work, but can you prove that this does not introduce inconsistencies with the model?
It would imply to deal with potentially infinitely large projection paths.
This feels like you are adding an abstraction layer for exactly this pattern. Without this we would have the simple model of that everything which is a reference is a root object.
| @@ -136,11 +136,18 @@ static inline bool isCastProjectionKind(ProjectionKind Kind) { | |||
| /// that immediately contains it. | |||
| /// | |||
| /// This lightweight utility maps a SIL address projection to an index. | |||
| /// | |||
| /// project_box does not have a projection index. At the SIL level, the box | |||
| /// storage is considered part of the same object as the. The box projection is | |||
| // only used by visitors that process access projections; once the accessed | ||
| // address is reached, they are no longer relevant. | ||
| template <typename UseDefVisitor> | ||
| class AccessPhiVisitor |
There was a problem hiding this comment.
I'm not a big fan of these visitors. It might be satisfying from a code-aesthetic point of view, but it adds complexity which is hard to understand. Each visitor has its own state and the algorithm, how all this works, is obfuscated.
I would prefer an old-fashioned, more functional, style for implementing the use-def analysis.
|
|
||
| // Find the origin of an access while skipping projections and casts and | ||
| // handling phis. | ||
| template <typename Impl> |
There was a problem hiding this comment.
... and templates add another level of complexity
| // AccessPath | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| bool AccessPath::contains(AccessPath subPath) const { |
There was a problem hiding this comment.
In contrast: "contains" and "mayOverlap" are beautiful - simple and easy to understand!
| // If subpaths are disjoint, they do not overlap regardless of offset. | ||
| if (!pathNode.node->isPrefixOf(otherPath.pathNode.node) | ||
| && !otherPath.pathNode.node->isPrefixOf(pathNode.node)) { | ||
| return true; |
There was a problem hiding this comment.
Shouldn't this be "return false"?
lib/SIL/Utils/MemAccessUtils.cpp
Outdated
| if (storage.getKind() == AccessedStorage::Class) { | ||
| pathIndices.push_back(AccessPath::Index::forSubObjectProjection( | ||
| storage.getPropertyIndex())); | ||
| } |
atrick
force-pushed
the
add-accesspath
branch
from
d2235e6 to
eb5f956
Compare
|
@gottesmm In order to stage in new verification, load-borrow immutability gets disabled in this PR but immediately reenabled here: https://github.com/atrick/swift/commits/rewrite-loadborrowchecker |
|
@swift-ci test |
|
@swift-ci test source compatibility |
|
@swift-ci benchmark |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
Build failed |
atrick
force-pushed
the
add-accesspath
branch
from
eb5f956 to
d279012
Compare
|
@swift-ci test |
|
@swift-ci test source compatibility |
|
@atrick that's a huge patch. If you are going to disable it, now that if anything creeps in, you need to fix. |
|
@gottesmm I've actually been waiting to land this PR until I know all the subsequent functionality, including the load-borrow-checker rewrite will pass all testing and SCK. So the only reason to disable load borrow checking here is to break up the functionality and allow the bots to churn through one piece at a time. |
|
Sure. I am cool with it. I am just saying if something sneaks in... its on you to fix = p. |
...and avoid reallocation. This is immediately necessary for LICM, in addition to its current uses. I suspect this could be used by many passes that work with addresses. RLE/DSE should absolutely migrate to it.
Change ProjectionIndex for ref_tail_addr to std::numeric_limits<int>::max(); This is necessary to disambiguate the tail elements from ref_element_addr field zero.
To clarify and unify logic, improve precision, and behave consistently with other code that does the same thing.
Things that have come up recently but are somewhat blocked on this: - Moving AccessMarkerElimination down in the pipeline - SemanticARCOpts correctness and improvements - AliasAnalysis improvements - LICM performance regressions - RLE/DSE improvements Begin to formalize the model for valid memory access in SIL. Ignoring ownership, every access is a def-use chain in three parts: object root -> formal access base -> memory operation address AccessPath abstracts over this path and standardizes the identity of a memory access throughout the optimizer. This abstraction is the basis for a new AccessPathVerification. With that verification, we now have all the properties we need for the type of analysis requires for exclusivity enforcement, but now generalized for any memory analysis. This is suitable for an extremely lightweight analysis with no side data structures. We currently have a massive amount of ad-hoc memory analysis throughout SIL, which is incredibly unmaintainable, bug-prone, and not performance-robust. We can begin taking advantage of this verifably complete model to solve that problem. The properties this gives us are: Access analysis must be complete over memory operations: every memory operation needs a recognizable valid access. An access can be unidentified only to the extent that it is rooted in some non-address type and we can prove that it is at least *not* part of an access to a nominal class or global property. Pointer provenance is also required for future IRGen-level bitfield optimizations. Access analysis must be complete over address users: for an identified object root all memory accesses including subobjects must be discoverable. Access analysis must be symmetric: use-def and def-use analysis must be consistent. AccessPath is merely a wrapper around the existing accessed-storage utilities and IndexTrieNode. Existing passes already very succesfully use this approach, but in an ad-hoc way. With a general utility we can: - update passes to use this approach to identify memory access, reducing the space and time complexity of those algorithms. - implement an inexpensive on-the-fly, debug mode address lifetime analysis - implement a lightweight debug mode alias analysis - ultimately improve the power, efficiency, and maintainability of full alias analysis - make our type-based alias analysis sensistive to the access path
Add a flag: enable-accessed-storage-dump-uses
Add AccesssedStorage::compute and computeInScope to mirror AccessPath. Allow recovering the begin_access for Nested storage. Adds AccessedStorage.visitRoots().
Compute 'isLet' from the VarDecl that is available when constructing AccessedStorage so we don't need to recover the VarDecl for the base later. This generally makes more sense and is more efficient, but it will be necessary when we look past class casts when finding the reference root.
A rewrite is ready and will be merged ASAP.
atrick
force-pushed
the
add-accesspath
branch
from
69ca2c6 to
5e0c8f9
Compare
|
@swift-ci smoke test |
Things that have come up recently but are somewhat blocked on this: