[SIL] NFC: Simplify SILVTable and save 8 bytes per SILVTable #32238
Conversation
There was a problem hiding this comment.
This makes the compiler non-deterministic.
For example, when iterating over the vtables for serialization.
The general rule is: never iterate over a map/set of pointers, except it can be 100% proven that the iteration order does not effect any compilation result.
|
Ya, I just figured out why the ref count was non-zero and now the test suite is blowing up as you predicted. Nevertheless, sort() can solve this, no? Or is important that vtables be emitted in source order? |
|
It doesn't necessarily be in source order. But it needs to be deterministic. |
|
So why do we have |
|
-emit-sorted-sil is to get some more consistent output in textual SIL, but it's not to fix non-determinism in the compiler. What would be the sort predicate for vtables? Beside that, providing an iteration API for something which is a non-deterministic map under the hood, is extremely dangerous. It's very easy to misuse this API and introduce non-determinism. |
|
Fair enough. I'm observing your point now in the test suite. In any case, we can still save memory and avoid pointer chasing if we don't use an intrusive list, so I'll simplify this patch. |
davezarzycki
changed the title
|
@swift-ci please test |
|
Build failed |
|
Build failed |
include/swift/SIL/SILModule.h
Outdated
| vtable_iterator vtable_begin() { return getVTableList().begin(); } | ||
| vtable_iterator vtable_end() { return getVTableList().end(); } | ||
| vtable_const_iterator vtable_begin() const { return getVTableList().begin(); } | ||
| vtable_const_iterator vtable_end() const { return getVTableList().end(); } | ||
| iterator_range<vtable_iterator> getVTables() { |
There was a problem hiding this comment.
I don't think we still need both of getVTableList() and getVTables().
I would keep
VTableListType getVTables() { return llvm::ArrayRef<SILVTable*>(vtables); }
| @@ -178,7 +178,7 @@ class SILModule { | |||
| llvm::DenseMap<const ClassDecl *, SILVTable *> VTableMap; | |||
|
|
|||
| /// The list of SILVTables in the module. | |||
| VTableListType vtables; | |||
| std::vector<SILVTable*> vtables; | |||
There was a problem hiding this comment.
Maybe we could use an llvm::MapVector here, so that we get both stable order and efficient lookup?
There was a problem hiding this comment.
I just looked at the llvm::MapVector implementation. It's less time and space efficient than what this pull request is proposing (in short: "map of keys to indices into a vector of key/value pairs"). It's also less convenient because the std::vector is key/value pairs when all we need/want is an array of values.
|
@eeckstein – I've made the getVTableList() -> getVTables() change you requested. @swift-ci please test |
|
Build failed |
|
Build failed |
|
Thanks. I'm going to hold out on merging this while I make a last ditch effort to get the build czars to fix last nights erroneous merge "correctly". |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
We were not using the primary benefits of an intrusive list, namely the ability to insert or remove from the middle of the list, so let's switch to a plain vector. This also avoids linked-list pointer chasing.
|
@swift-ci please test |
|
Build failed |
|
Build failed |
EDITED
We are not using the primary benefits of an intrusive list, namely the ability to insert or remove from the middle of the list, so let's switch to a plain vector. This also avoids linked-list pointer chasing.
OLD TEXT
I noticed that vtables were being tracked redundantly by SILModule and nothing seems to be using the intrusive list aspect of SILVTable, so I tried just converging everything onto the DenseMap. This almost works, but then the destructor for SILModule blows up when it finds a function with a non-zero reference count. This surprised me.
Am I missing something obvious?