RFR: 8315066: Add unsigned bounds and known bits to TypeInt/Long [v2]

[Quan Anh Mai]

On Mon, 25 Sep 2023 07:30:55 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> @TobiHartmann Thanks a lot for taking a look at this patch. Regarding your questions:
>> 
>> - I don't think splitting these 2 would ease reviewing. Since individual changes are small and mostly contained in various `Value` methods, splitting unsigned bounds and known bits would likely to result in the need to review the same thing twice.
>> - Maybe I miss some important context, but IIUC, the current implementation is more hacky. Since a value set and its dual set are fundamentally different concepts, what is currently done is that we find a common representation space for both of them and a binary operation on that space such that the union operations on both the value sets and the dual sets map to that binary operation when we map the 2 sets onto the representation space. In a more detailed explanation, the value set of is the set of `x` such that `lo <= x && x <= hi`, the dual set is the set of `x` such that `x > hi || x < lo`, the representation space is `(x, y)` and the mapping for the 2 sets are `(lo, hi) -> (lo, hi)` and `(lo, hi) -> (hi, lo)`, respectively. Incidentally, the binary operation popping up is `(min(x1, x2), max(y1, y2))`. This binary operation, unfortunately, is ad-hoc and may not be trivial for more complex sets.
>> - Yes the sign of the result is determined by its sign bit so this patch does solve that issue.
>> - I think the patch fully covers John's prototype since the prototype does not concern with unsigned bounds as well as the interaction between signed bounds and known bits. The bit calculation of add and sub is really impressive, though.
>> 
>> Thanks a lot,
>> Quan Anh
>
>> I don't think splitting these 2 would ease reviewing
> 
> Okay, thanks for the details! Let's leave it as is then.
> 
>> Yes the sign of the result is determined by its sign bit so this patch does solve that issue.
> 
> I closed [JDK-8311597](https://bugs.openjdk.org/browse/JDK-8311597) as duplicate.
> 
>> I think the patch fully covers John's prototype
> 
> Okay, I leave it to @rose00 to decide if [JDK-8001436](https://bugs.openjdk.org/browse/JDK-8001436) should be closed as duplicate then.
> 
>> it simply moves the complexity and burden of correctness from xmeet where branching on the _dual field is pretty straightforward to xdual where this representation is a fair leap of faith
> 
> Thanks for summarizing. Right, I agree that in this case it's much cleaner to special case the dual types but I don't think that would apply in general to all the types (for example, `TypePtr::meet_instptr`). So this is all about consistency of new changes with existing code and I would prefer consistency over special casing. If we identify improvements to the existing representation of types, that should be done in a separate change and consistently over all types. I'm curious what other reviewers think though.

@TobiHartmann I have tried your suggestion, it turns out that the interdependence between constraints makes it difficult to tolerate ambiguity in type presentations. Without knowing whether the context is a dual type or not, it is really hard to normalise the constraints, verify them and normalise the widen parameter. What do you think? Thanks.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/15440#issuecomment-1771520166