[sil-mem2reg] Add a simple scope data structure and use it to simplify some code. #36913
Conversation
…y some code. We have for a long time talked about creating a scope like data structure for use in the SILOptimizer. The discussion was whether or not to reuse the infrastructure in SILGen that does this already. There were concerns about doing so since the code in the SILOptimizer and SILGen can work differently. With that in mind, I added a small AssertingScope class and built on top of that a composition SIL level class called SILOptScope that one can use to add various cleanups. One is able to both destructively pop at end of scope and pop along early exits. At an implementation level, I kept it simple and: 1. Represented a scope as a stack of Optional<Cleanup> which are just a wrapper around a std::function. The Optional is so that we can invalidate a cleanup. 2. Based all of these scopes around the idea that the user of the scope must invalidate the scope by hand. If not, the scope object will assert at the end of its RAII scope. 3. Rather than creating a whole class hierarchy, I just used std::function closures to keep things simple.
|
@swift-ci test |
|
Build failed |
|
@swift-ci test macOS platform |
| /// on Optional has value. | ||
| void invalidateCleanup(unsigned cleanupIndex) { | ||
| assert(!didPop && "Should not invalidate once popped?!"); | ||
| assert(cleanupIndex < cleanups.size()); |
There was a problem hiding this comment.
nit: I don't think you need to explicitly bounds-check a data structure that's already bounds checked.
There was a problem hiding this comment.
I'll fix this in a follow on commit.
| } | ||
|
|
||
| /// Can not be called once the scope is moved. | ||
| void nonDestructivePop(Args... args) const { |
There was a problem hiding this comment.
For future consideration. It seems like the point of nonDestructivePops would be to copy the scope without making a physical copy. So it would be more robust/usable to allow scopes to be either copied or chained with multiple scopes sharing the cleanup vector. Then popping is only destructive when you're the original or unique owner of those cleanups. I don't know if copying (with CoW behavior) or chaining with shared cleanups would make more sense in practice.
There was a problem hiding this comment.
I would need to think about this. The point of this is to enable one to along early exiting paths pop all current scoped things.
In SILGen, the scope data structure allows for a shared cleanup vector. I think that would be a great add-on. I was trying to keep this simple.
| return pushDestroyValue(copy); | ||
| } | ||
|
|
||
| ScopedValue pushEndBorrow(SILValue value) { |
There was a problem hiding this comment.
It's slightly questionable as to why you'd bother creating a ScopedValue when there's no cancelable cleanup, but seems fine. Presumably you could assert that the scope is still active when the ScopedValue is used, but I don't know if there's any intention to actually do that.
There was a problem hiding this comment.
the reason why I did this is that in SILGen we found it interesting to be able to write APIs against ManagedValue that both had a cleanup/did not have a cleanup without having to change the currency type and thus have multiple APIs.
As suggested by @atrick in review feedback in apple#36913.
We have for a long time talked about creating a scope like data structure for
use in the SILOptimizer. The discussion was whether or not to reuse the
infrastructure in SILGen that does this already. There were concerns about doing
so since the code in the SILOptimizer and SILGen can work differently.
With that in mind, I added a small AssertingScope class and built on top of that
a composition SIL level class called SILOptScope that one can use to add various
cleanups. One is able to both destructively pop at end of scope and pop along
early exits.
At an implementation level, I kept it simple and:
around a std::function. The Optional is so that we can invalidate a cleanup.
invalidate the scope by hand. If not, the scope object will assert at the end
of its RAII scope.
closures to keep things simple.