-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8370914: C2: Reimplement Type::join #28051
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back qamai! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@merykitty The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive work @merykitty!
It would be good if you could explain how your new lattice definition compares to our current definition. Especially, in regard to the completeness of join, which we currently get for free from symmetry and a complete meet, and associativity and distributivity and how we have to implement the relation in Value() to keep those properties.
While I see that your definition is potentially easier to reason about, I think that you first need to convince everyone that your definition has no significant drawbacks to what we currently have simply because this change is so fundamental.
| }; | ||
|
|
||
| void Type::check_symmetrical(const Type* t, const Type* mt, const VerifyMeet& verify) const { | ||
| void Type::check_symmetrical(const Type* t1, const Type* t2, VerifyMeet& verify) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need a rename ;)
|
@mhaessig Thanks for taking a look.
The thing is that it is easier and more intuitive to think of a
It is not free, though. It requires us to prove that the type representation is symmetric and implement Another issue is that the existence of dual types doubles the set of values that can participate in a meet. For example, in |
|
Furthermore, I believe our current representation is also not fully symmetric. For example, when meeting 2 |
|
I'm not trying to argue for symmetry. All I am trying to say is that our current type lattice has some properties, mainly associativity and distributivity, that let us apply meets and joins in arbitrary order and reach the same optimal result. Also, we need a lattice, otherwise not every pair of types have a meet or join, which would be quite cumbersome/impossible to use. So I want to understand mathematically how you uphold these properties. |
|
For associativity and distributivity, I think we need to check for comformance of each implementation. Luckily, if for a
We actually rarely need to meet/join unrelated types (e.g joining a float and an int). As a result, disallowing those operations may help us catch unwanted errors. We still have |
Hi,
Currently,
Type::joinis implemented usingType::dual. The idea seems to be that the dual of a join would be the meet of the duals of the operands. This helps us avoid the need to implement a separate join operation. The comments also discuss the symmetry of the join and the meet operations, which seems to refer to the various fundamental laws of set union and intersection.However, it requires us to find a representation of a
Typeclass that is symmetric, which may not always be possible without outright dividing its value set into the normal set and the dual set, and effectively implementing join and meet separately (e.g.TypeIntandTypeLong).In other cases, the existence of dual types introduces additional values into the value set of a
Typeclass. For example, a pointer can be a nullable pointer (BotPTR), a not-null pointer (NotNull), a not-null constant (Constant), a null constant (Null), an impossible value (TopPTR), andAnyNull? This is really hard to conceptualize even when we know thatAnyNullis the dual ofNotNull. It also does not really work, which leads to us sprinklingabove_centerlinechecks all over the place. Additionally, the number of combinations in a meet increases quadratically with respect to the number of instances of aType. This makes the already hard problem of meeting 2 complicated sets a nightmare to understand.This PR reimplements
Type::joinas a separate operation and removes most of thedualconcept from theTypeclass hierachy. There are a lot of benefits of this:Typeclasses can be made easier. Instead of thinking about type lattices and how the representation places theTypeobjects on the lattices, it is much easier to conceptualize a join when we think aTypeas a set of possible values.ptr()value when aTypeInstPtrparticipating in a meet/join operation, there are only 3 left (AnyNullis non-sensical andTopPTRmust be anAnyPtr). This, in turns, reduces the number of combinations in a meet/join from 25 to 9, making it much easier to reason about.This PR also tries to limit the interaction between unrelated types. For example, meeting and joining of a float and an int seem to happen only when we try to do those operations on the array types of those types. Limiting these peculiar operations to the places they happen makes it easier to catch unexpected interactions. It also helps us avoid sprinkling a bunch of cases in each meet and join method.
Future work:
xmeetmethods too much. There is a lot of room for simplification since the number of operand combinations has decreased significantly.dualsuch asTypeInt::_dualandTypePtr::above_centerline.TypeOopPtrcan be made, orTypePtrcannot be made withBotPTR.Please take a look and leave your reviews, thanks a lot.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28051/head:pull/28051$ git checkout pull/28051Update a local copy of the PR:
$ git checkout pull/28051$ git pull https://git.openjdk.org/jdk.git pull/28051/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28051View PR using the GUI difftool:
$ git pr show -t 28051Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28051.diff
Using Webrev
Link to Webrev Comment