SILCombiner: Clean up the concrete -> existential peephole a bit #7917
Conversation
|
@swift-ci Please smoke test |
|
@slavapestov Are the dynamic self checks really not needed anymore (just double checking)? |
|
I need to add a test case before merging but it looks like they were only needed back when the combiner did devirt. The devirtualizer knows to insert the right bitcasts when I tested this manually. |
Use Substitution::subst() to replace the opened existential with the concrete type. I don't have a test case, but I think the old code would have failed if a non-Self substitution also contained the opened existential, which could happen after generic inlining. Also, it looks like the guard against devirtualizing methods returning Self is no longer necessary, because the devirtualizer can insert the necessary casts. In any case, the check was incorrect because we now allow calling methods on existentials that return Self as part of another type in covariant position, such as Optional<Self> or `() -> Self`.
|
@gottesmm I added the test. Actually I realized it would be nice if peephole was be generalized so that all occurrences of the opened existential are replaced with the concrete type, not just witness_method and apply. Right now, we still alloc_stack an opened existential and cast the result, instead of just alloc_stack'ing the concrete type, which results in less efficient code in IRGen. I think now with the opened archetype use tracking, we should be able to implement this pretty easily. |
slavapestov
force-pushed
the
sil-combiner-cleanup
branch
from
b9567fa to
44f2397
Compare
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
@slavapestov Thanks! I just remember a lot of bugs coming up due to issues related to that. Just didn't want to have to fix them again! |
|
If they come up again, we'll fix them properly instead of just disabling the optimization. ;-) |
No description provided.