8315066: Add unsigned bounds and known bits to TypeInt/Long #17508
Conversation
|
👋 Welcome back qamai! A progress list of the required criteria for merging this PR into |
|
@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. |
|
@TobiHartmann @eme64 I have extracted a part of #15440, could you take a look when you have time, please? Thanks a lot for your help. |
There was a problem hiding this comment.
@merykitty I just had a quick look. Thanks for spitting out parts and making it more reviewable that way! Since John Rose is generally excited (#15440 (comment)), I'll now put in a bit more effort into reviewing this.
Thanks for adding some gtests.
I would really like to see some IR tests, where we can see that this folds cases, and folds them correctly.
And just some general java-code correctness tests, which test your optimizations in an end-to-end way.
I have a general concern with the math functions. They have quite a few arguments, often 5-10. And on average half of them are passed as a reference. Sometimes it is hard to immediately see which are the arguments that will not be mutated, and which are return values, and which are both arguments and return values, which are simply further constrained/narrowed etc.
I wonder if it might be better to have types like:
SRange<int/long> {lo, hi}
URange<int/long> {lo, hi}
KnownBits<int/long> {ones, zeros}
Make them immutable, i.e. the fields are constant.
Then as function parameters, you always pass in these as const, and return the new values (possibly in some combined type, or a pair or tuple or whatever).
I think it would make the code cleaner, have fewer arguments, and a bit easier to reason about when things are immutable.
Plus, then you can put the range-inference methods inside those classes, you can directly ask such an object if it is empty etc. You could for example have somelthing like:
SRange::constrained_with(KnownBits) -> returns SRange. Basically I'm asking for the code to be a little more object-oriented, and less C-style ;)
|
@eme64 Thanks a lot for your reviews, I hope that I have addressed your concerns. Regarding IR tests, I don't think I can come up with any as there is no node taking advantage of the additional information yet. |
There was a problem hiding this comment.
I took a quick look through the patch, this is really impressive :)
Early last year I had an attempt at the same idea (extremely rough patch, sorry), where I went with the approach of using a 2-bit value for each bit position to represent 0, 1, BOTTOM, and TOP. My general idea was to create a boolean lattice so that the meet() and dual() operations were easier to implement, before I realized how difficult reasoning about multiple constraints in the meet and dual operations was. I think your idea of marking the dual makes more sense and is cleaner, especially with how the constraints interact.
|
@jaskarth Thanks for looking into this patch. I have tried not having an explicit |
I forgot that |
|
@merykitty This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
Keep alive. |
|
@merykitty This change is no longer ready for integration - check the PR body for details. |
|
@merykitty This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@merykitty This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
|
/open |
|
@merykitty This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@eme64 It would be great if you can continue reviewing this PR, please. |
There was a problem hiding this comment.
@merykitty I've always had this on the list. I'm a little tired today, so won't tough the math heavy stuff, but I have some other suggestions. I'll try to put in a bit of time multiple time a week to get through this eventually.
| if (!nbits._is_result_consistent) { | ||
| return SimpleCanonicalResult<U>::make_empty(); | ||
| } | ||
| AdjustResult<RangeInt<U>> nbounds{true, true, bounds}; |
There was a problem hiding this comment.
nbounds: what is the n here?
Since you are doing this iteratively, I think nbits -> current_bits, and nbounds -> current_bounds would be more explicit.
Alternatively, you rename the inputs to old_..., and just call the ones you keep updating bits and bounds. Up to you.
There was a problem hiding this comment.
It is indeed "new", I renamed it to canonicalized_bits and canonicalized_bounds
| if (!nbounds._progress || !nbounds._is_result_consistent) { | ||
| return {nbounds._is_result_consistent, nbounds._result, nbits._result}; |
There was a problem hiding this comment.
That sounds a little scary to me:
It sounds like we could have inconsistent results: !nbounds._is_result_consistent and then simply return the nbounds._result. Is that ok, or just confusing wording?
I think again here empty would be our friend: returning an empty result is more sane than returning an inconsistent one 😅
There was a problem hiding this comment.
If the _present flag is false then it does not matter what is in the results.
|
@eme64 I have added exhaustive tests that scan all the possible inputs for |
There was a problem hiding this comment.
Wow, this stuff is really impressive!
I'm dedicated to doing this bit by bit, as it takes a lot of concentration and time.
I'm leaving you with a list of comments, and hope we can interactively get there 😊
Thanks for your patience! If I'm too slow, and there are other reviewers that are willing to reviewer faster, then someone else can take it over, but otherwise I will just keep on reviewing at my speed 😅
|
@eme64 Thanks a lot for your effort, don't worry and just take your time. |
|
@eme64 Ping |
|
@merykitty this pull request can not be integrated into git checkout unsignedbounds
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
|
@merykitty Sorry, I've been sick for a week and only just catching up with things again slowly... |
openjdk
bot
removed
the
merge-conflict
Hi,
This patch adds unsigned bounds and known bits constraints to TypeInt and TypeLong. This opens more transformation opportunities in an elegant manner as well as helps avoid some ad-hoc rules in Hotspot.
In general, a
TypeInt/Longrepresents a set of valuesxthat satisfies:x s>= lo && x s<= hi && x u>= ulo && x u<= uhi && (x & zeros) == 0 && (x & ones) == ones. These constraints are not independent, e.g. an int that lies in [0, 3] in signed domain must also lie in [0, 3] in unsigned domain and have all bits but the last 2 being unset. As a result, we must canonicalize the constraints (tighten the constraints so that they are optimal) before constructing aTypeInt/Longinstance.This is extracted from #15440 , node value transformations are left for later PRs. I have also added unit tests to verify the soundness of constraint normalization.
Please kindly review, thanks a lot.
Testing
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17508/head:pull/17508$ git checkout pull/17508Update a local copy of the PR:
$ git checkout pull/17508$ git pull https://git.openjdk.org/jdk.git pull/17508/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17508View PR using the GUI difftool:
$ git pr show -t 17508Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17508.diff
Using Webrev
Link to Webrev Comment