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 now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 54 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@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 |
|
@eme64 Thanks for not forgetting me :) I have answered your reviews. |
There was a problem hiding this comment.
@merykitty Ok then, thanks for working through all this with me!
I'm really happy with the result now, and I feel like I learned a lot from you 😊
@vnkozlov You might want to have another pass over it, as we reworked quite a bit.
|
This looks fine to me but to be on safe side lets push it into JDK 26 when it is forked. |
|
@vnkozlov I have merged this branch with master, can you run your tests and approve the changes, please? |
I submitted testing. |
|
Hi @merykitty, I guess the reason is that the existing TypeInt lattice only captures signed value ranges. All non-top Integral types must ensure that type._lo must never be greater than type._hi, in case of an unsigned range value wraps around after MAX_INT, thus it may result into breaking the value range invariant as in such a case type._hi < type.lo. The point of associating multiple lattice values i.e. signed, unsigned, and known bits, with a given integral type is to have a generic implementation where at any given point all three lattices are in sync, and canonicalization ensures this. But under practical scenarios, KnownBits and signed lattice points will be used most since Java type system inherently supports only signed types. |
|
@merykitty , your comments on the following understand will be helpful Q. Is it ok to keep a bool flag in a value which signifies that bounds hold unsigned values? A. Java primitive types are inherently signed, and C2 compiler represents all the integral types i.e., byte, short, int through TypeInt by simply constraining the value range, and long using TypeInt. For float C2 type system creates Type::FLOAT, and for double Type::DOUBLE, unlike the integral type, these two types do not record the actual value range bound. floating point constants C2 creates a different type, i.e. TypeF, and for double constant TypeD. Currently, the scope of the unsigned type is limited to comparison, multiplication, and division operations. Since the signed and unsigned value ranges overlap hence keeping a flag with _lo and _hi shoud suffice, new scheme accepts bounds of signed and unsigned value ranges then finds the effective value range, this allows user to feed any random signed and unsigned value ranges to a TypeInt and then let compiler find the effective value range by canonicalization. A TypeInt is only useful after canonicalization, it mimics the job of constructor where a newly allocated object is usable after it pushed though constructor, likewise a TypeInt accepts different signed and unsigned bounds but its only usable after normalization which computes effective value range and after normalization, signed, unsigned and knownbits are in sync. During the dataflow analysis, flow functions associated with different operators may modify the value ranges (signed or unsigned), this will tigger re-normalization, in other cases flow analysis may transfer only the known bits which is then used to prune the value ranges, at any given point, signed / unsigned and knownbit should be in sync, else the type is inconsistent and not usable, iterative canonicalization ensures this. Thus, to be flexible in implementation, keeping a separate value range for unsigned bounds is justified, but may not add huge value as all Java primitive types are inherently signed, and mixing of signed and unsigned operations in type flow is not possible. The whole idea of keeping the implementation flexible with unsigned bounds is based on an assumption that during data flow any of the lattices associated with an integral types TypeInt or TypeLong i.e. unsigned bounds, signed bounds or known bits may change. In practice only known bits (bit level df) and signed bounds may be usable a flag with signify a bound is unsigned may suffice, associating 3 lattice points with TypeInt is on account of flexibility, it may facilitate injecting opaque IR with unsigned bounds in SoN by optimization passes and then let the type canonicalization and iterative data flow analysis do the magic. |
|
@jatin-bhateja Thanks a lot for your suggestion. I will address your concerns below: In addition to being additional information, unsigned bounds make it easier for canonicalization. This is because bits are inherently unsigned, canonicalizing bits and unsigned bounds together is an easier task than canonicalize bits and signed bounds. I think it is also beneficial to be consistent, keeping a boolean to signify unsigned bounds splits the set of all I don't think it suffices to think that Java integral types are inherently signed. Bitwise and, or, xor, not are inherently unsigned. Left shift is both signed and unsigned, right shift has both the signed variant and the unsigned variant. Add, sub, mul are both signed and unsigned. There are only cmp, div, mod, toString and conversions that are signed, but we have methods to do the unsigned variants for all of them, |
"As a result, I think it is better to think of and Sounds good, I think since now integral types has multiple lattics point associated with it, and all our existing Value transforms are based on signed value ranges, we will also need to extend existing value transforms to make use of KnownBits, for that we will need to extend newly added KnownBits class to support other operations which accepts KnownBits inputs operates over them as per the operation semantics and returns a new KnownBits which post canonicalization also adjusts other lattice points i.e. signed and unsigned ranges. All in all, IIUC going forward we are planning to perform bit-level data flow analysis using KnownBits and then apply its result over signed / unsigned ranges, this will ensure all existing Value transforms, which are based on value ranges, continue to be useful and KnownBits analysis will sharpen the ranges. |
There was a problem hiding this comment.
Great work @merykitty , it was a pleasure to study your patch and refresh some of low level details of bit level data flow analysis.
LGTM,
Best Regards
|
@jatin-bhateja The goal is to make it possible to run gtests with any size integral types (e.g. 4 bit), so that we can efficiently test the corresponding value optimizations. @merykitty Did you already file a JBS issue for that? So when we refactor, we should try to create methods that take in types, and not nodes. That would allow us to generate types in the gtest, and get a type back. We can do all sorts of enhanced verification that way. For example, we can feed in wider and narrower types as inputs, and then expect that narrower types lead to narrower outputs, and wider types to wider outputs. If constants are fed in, then constants should come out. etc. This would really allow us to exhaustively verify for all sorts of ranges and bit patterns - at least for the smaller types (e.g. 4 bits). |
|
So there is indeed a lot of extension work to do, like @jatin-bhateja said. But we can use that to also refactor the code for testability. |
Thanks @eme64 for clarifying that is what I assumed, currently all value transforms are based on signed ranges and the role of KnownBits is restricted to canonicalization, which sync up the value ranges to known bits, but it will add much more value if we do the reverse where in data flow analysis is performed over KnownBits and then canonicalization adjusts the value ranges accordingly. So, adding a new gtest for each new operation is the right next step. |
|
@eme64 I have created https://bugs.openjdk.org/browse/JDK-8359149 |
|
@merykitty |
|
/integrate |
|
Going to push as commit 991097b.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@merykitty Pushed as commit 991097b. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
@merykitty Thanks for the attribution 😊 Congrats on shipping it 🚀 Looking forward to the improvements that come from this! |
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
Contributors
<[email protected]>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