RFR: 8350896: Integer/Long.compress gets wrong type from CompressBitsNode::Value [v8]

[Emanuel Peter]

On Fri, 30 May 2025 17:43:27 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Review comments resolutions
>
> We can further constrain the value range bounds of bit compression and expansion once PR #17508 gets integrated. For now, I have developed the following draft demonstrates bound constraining with KnownBitLattice.
> 
> 
> //
> // Prototype of bit compress/expand value range computation
> // using KnownBits infrastructure.
> //
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <stdint.h>
> #include <assert.h>
> 
> template<class U,class S>
> class KnownBitsLattice {
>   private:
>     U zeros;
>     U ones;
> 
>   public:
>     KnownBitsLattice(U lb, U ub);
> 
>     U getKnownZeros() {
>       return zeros;
>     }
> 
>     U getKnownOnes() {
>       return ones;
>     }
> 
>     long getKnownZerosCount() {
>       uint64_t count = 0;
>       asm volatile ("popcntq %1, %0 \n\t" : "=r"(count) : "r"(zeros) : "cc");
>       return count;
>     }
> 
>     long getKnownOnesCount() {
>       uint64_t count = 0;
>       asm volatile ("popcntq %1, %0 \n\t" : "=r"(count) : "r"(ones) : "cc");
>       return count;
>     }
> 
>     bool check_voilation() {
>       // A given bit cannot be both zero or one.
>       return (zeros & ones) != 0;
>     }
> 
>     bool is_MSB_KnownOneBitsSet() {
>       return (ones >> 63) == 1;
>     }
> 
>     bool is_MSB_KnownZeroBitsSet() {
>       return (zeros >> 63) == 1;
>     }
> };
> 
> template<class U, class S>
> KnownBitsLattice<U,S>::KnownBitsLattice(U lb, U ub) {
>   // To find KnownBitsLattice from a given value range
>   // we first find the common prefix b/w upper and lower
>   // bound, we then concertize known zeros and ones bit
>   // based on common prefix.
>   // e.g.
>   //    lb = 00110001
>   //    ub = 00111111
>   //    common prefix = 0011XXXX
>   //    knownbits.zeros = 11000000
>   //    knownbits.ones = 00110000
>   //
>   // conversely, for a give knownbits value we can find
>   // lower and upper value ranges.
>   // e.g.
>   //      knownbits.zeros = 0x00010001
>   //      knownbits.ones  = 0x10001100
>   //  range.lo = knownbits.ones, this is because knownbits.ones are
>   //                             guaranteed to be one.
>   //  range.hi = ~knownbits.zeros, this is an optimistic upper bound
>   //                               which assumes all unset knownbits.zero
>   //                               are ones.
>   // Thus in above example,
>   // range.lo = 0x8C
>   // range.hi = 0xEE
> 
>   U lzcnt = 0;
>   U common_prefix = lb ^ ub;
>   asm volatile ("lzcntq %1, %0 \n\t" : "=r"(lzcnt) : "r"(common_prefix) : "cc");
>   U common_prefix_mask =  lzcnt == 0 ? 0xFFFFFFFFFFFFFFFFL : ~((1ULL << (64 - lzcnt)) - 1);
>   zeros = (~lb) & common_prefix_mask;
>   ones = (lb) & c...

@jatin-bhateja I think we are making progress, it seems to me now that the VM code is correct, at least as far as I can tell with visual inspection.

We are still missing some additional tests, as I have asked for a few times already:
https://github.com/openjdk/jdk/pull/23947#issuecomment-2853896251

We should do something like this:

public static test(int mask, int src) {
  mask = Math.max(CON1, Math.min(CON2, mask));
  src = Math.max(CON2, Math.min(CON4, src));
  result = Integer.compress(src, mask);
  int sum = 0;
  if (sum > LIMIT_1) { sum += 1; }
  if (sum > LIMIT_2) { sum += 2; }
  if (sum > LIMIT_3) { sum += 4; }
  if (sum > LIMIT_4) { sum += 8; }
  if (sum > LIMIT_5) { sum += 16; }
  if (sum > LIMIT_6) { sum += 32; }
  if (sum > LIMIT_7) { sum += 64; }
  if (sum > LIMIT_8) { sum += 128; }
  return new int[] {sum, result};
}


You could do the same pattern for `expand` too.
Then you pick random values using `Generators.java` for all the `CON` and `LIMIT`.
If we somehow produce a bad range, then the limit checks could constant fold wrongly, and then the `sum` would reflect this wrong result. Optimal would be to duplicate this pattern, and have one method that compiles, and one that runs in interpreter. That way, you can repeatedly call the methods with various `src` and `mask` values, and compare the output.

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

PR Comment: https://git.openjdk.org/jdk/pull/23947#issuecomment-2935548411