Space: Cache space decomposition & other refactoring #16937
Conversation
This is in preparation for making SpaceEngine an object, so also remove the other vals too.
|
@liufengyun it's quite a big change. I tried to explain what it includes, the reasons and present the changes in atomic diffs. But let me know what you think and how you feel about it. |
There was a problem hiding this comment.
Very nice improvement 🎉 I only have some minor style comments.
-
In 20026ec, it's not obvious why the change fixed the test. Adding a comment would help future maintenance.
-
Maybe trigger a bench test.
PS: I worried introducing cache would make maintenance harder, but looking at the code, I think it's still easy to understand and maintain.
| if isSubType(tp2, tp1) then b | ||
| else if canDecompose(a) then intersect(Or(decompose(a)), b) | ||
| else if isSubType(tp1, tp2) then a // problematic corner case: inheriting a case class | ||
| else intersectUnrelatedAtomicTypes(tp1, tp2)(b) |
There was a problem hiding this comment.
Minor: In comparison with previous cases, what's the consideration here to introduce the ascription a but not b in the pattern?
There was a problem hiding this comment.
I bind a because I need a Typ, not a Space, to decompose, while intersectUnrelatedAtomicTypes handles all spaces so I don't need to rebind the name.
dwijnand
force-pushed
the
space/object
branch
from
e76ab5d to
daef636
Compare
Firstly, make SpaceEngine an object, in line with many other parts of the compiler, moving the cache it had for
isSubspace onto Space instances themselves, also like many other parts of the compiler. This also just helps
code navigation - avoid navigating to the interface all the time...
Then add a cache for decompose, canDecompose and simplify, so the costly result isn't computed multiple times.
And finally, look to define canDecompose in terms of decompose, so the logic is shared.