RFR: 8351645: C2: ExpandBitsNode::Ideal hits assert because of TOP input

[Emanuel Peter]

On Mon, 2 Jun 2025 11:53:23 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

> Bugfix patch adds missing safe type access checks in Expand/Compress Ideal transforms.
> Test mentioned in the bug report has been included allong with the patch.
> 
> Kindly review.
> 
> Best Regards,
> Jatin

@jatin-bhateja Thanks for looking into this.

I just sanity checked the implementation of `compress` ... and the code there is exactly the same!

I just took the reproducer and replaced `expand` with `compress` ... and got another assert.


public class Test {
    public static long[] array_0 = fill(new long[10000]);                                                                                                                                                                                  
    public static long[] array_2 = fill(new long[10000]);

    public static long[] fill(long[] a) {
        for (int i = 0; i < a.length; i++) {
            a[i] = 1;
        }
        return a;
    }

    public static long one = 1L;

    static final long[] GOLD = test();

    public static long[] test() {
        long[] out = new long[10000];
        for (int i = 0; i < out.length; i++) {
            long y = array_0[i] % one;
            long x = (array_2[i] | 4294967298L) << -7640610671680100954L;
            out[i] = Long.compress(y, x);
        }
        return out;
    }

    public static void main(String[] args) {
        for (int i = 0; i < 10_000; i++) {
            test();
	}

        long[] res = test();
        for (int i = 0; i < 10_000; i++) {
            if (res[i] != GOLD[i]) {
                throw new RuntimeException("value mismatch: " + res[i] + " vs " + GOLD[i]);
	    }
	}
    }
}


With: `java  -Xbatch -XX:CompileCommand=compileonly,Test::test* -XX:+StressIGVN -XX:RepeatCompilation=100 Test.java`

Can you please also fix that here, and add regression tests for `Integer/Long.compress`?

You should also update the PR title accordingly.

src/hotspot/share/opto/intrinsicnode.cpp line 196:

> 194:   Node* mask = in(2);
> 195:   if (bottom_type()->isa_int()) {
> 196:     if (mask->Opcode() == Op_LShiftI && phase->type(mask->in(1))->isa_int() && phase->type(mask->in(1))->is_int()->is_con()) {

Why not just check for `top` at the beginning of the function? Just like here:

  311 const Type* CompressBitsNode::Value(PhaseGVN* phase) const {
  312   const Type* t1 = phase->type(in(1));
  313   const Type* t2 = phase->type(in(2));
  314   if (t1 == Type::TOP || t2 == Type::TOP) {                                                                                                                                                                                                                                                                                                                                                                                           
  315     return Type::TOP;
  316   }

Of course in `Ideal` you would have to return `nullptr` instead, and wait for `Value` to clean it up.

That has the benefit that you only need to check it in one place, and then any new optimization we might add in the future does not also have to deal with `top`.

test/hotspot/jtreg/compiler/intrinsics/Test8351645.java line 1:

> 1: /*

I would put the test under `test/hotspot/jtreg/compiler/c2/gvn/TestExpandTopInput.java`

Because this is not per se about an intrinsic, more about the `gvn` optimization failing.

test/hotspot/jtreg/compiler/intrinsics/Test8351645.java line 28:

> 26:  * @bug 8351645
> 27:  * @summary C2: ExpandBitsNode::Ideal hits assert because of TOP input
> 28:  * @run main/othervm -Xbatch -Xmx128m compiler.intrinsics.Test8351645

What is the reason for the flags here? Do you really need them? I guess `-Xbatch` could make sense, just to make sure the method is compiled.
And you did not need `-XX:CompileCommand=compileonly,Test::test*` to reproduce this, right? Just want to be sure that inlining is not somehow creating issues here.

test/hotspot/jtreg/compiler/intrinsics/Test8351645.java line 53:

> 51:             long y = array_0[i] % one;
> 52:             long x = (array_2[i] | 4294967298L) << -7640610671680100954L;
> 53:             out[i] = Long.expand(y, x);

Can you please also add a test for `Integer.expand`?
Because it seems that your fix addresses not just the `long` but also the `int` case, right?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25586#pullrequestreview-2895648186
PR Review Comment: https://git.openjdk.org/jdk/pull/25586#discussion_r2125911301
PR Review Comment: https://git.openjdk.org/jdk/pull/25586#discussion_r2125935617
PR Review Comment: https://git.openjdk.org/jdk/pull/25586#discussion_r2125916907
PR Review Comment: https://git.openjdk.org/jdk/pull/25586#discussion_r2125912495