RemoveUnusedModuleElements: Optimize indirect calls when the table is immutable #8107
Conversation
|
Measuring on Dart, I see a small benefit here. Probably it is small as other devirtualization etc. mechanisms already handle most things. |
|
friendly nonurgent ping @tlively |
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
Sorry, looks like I've had this comment pending for a while but never published it. Will take a fresh look as well.
src/ir/table-utils.h
Outdated
| // Whether we can assume that the initial contents are immutable. See the | ||
| // toplevel comment. | ||
| bool initialContentsImmutable = false; |
There was a problem hiding this comment.
I'm not sure which comment this is referring to, and I'm not sure just by reading this how initialContentsImmutable differs from mayBeModified.
There was a problem hiding this comment.
Ah, good point, I moved code but not all the dependent comments 😄 Fixed.
I also clarified how the properties tie into each other.
src/ir/table-utils.cpp
Outdated
| } | ||
| } | ||
| if (hasUnmodifiableTable) { | ||
| using TablesWithSet = std::unordered_set<Name>; |
There was a problem hiding this comment.
Let's do an early return here when !hasUnmodifiableTable to decrease the indentation below.
| ) | ||
| (table.set $table | ||
| (i32.const 7) | ||
| (ref.func $foo-not-in-table) |
There was a problem hiding this comment.
Maybe write a null here to show that the optimization is still inhibited.
There was a problem hiding this comment.
That would change the test: without taking a reference of this function, we can't verify that the reference matters. As the test says later down for that function
;; The reference taken of this function might be table.set'ed, so we can do
;; nothing here.
We could separate the reference from the table.set, put a null in the latter and drop the reference, I guess, but I'm not sure if that adds anything?
There was a problem hiding this comment.
We could separate the reference from the
table.set, put a null in the latter and drop the reference, I guess, but I'm not sure if that adds anything?
I think we should do this. It better demonstrates that we're not looking at precisely what is set, but rather that there is a set at all.
2 participants
To do this, refactor the table scanning code out of Directize into TableUtils. Then
when we see a CallIndirect in RemoveUnusedModuleElements, we no longer
automatically assume it could call anything whose reference was taken: if we see
the table is not modified, then only the table's known contents might be called.
That is, before: CallIndirect implied we could call anything of that type, in that
table. But also, we assumed other things might be written into the table at
runtime, so anything whose reference was taken was callable. After: We know
which tables do not have new entries written into them.