RFR: 8303466: C2: failed: malformed control flow. Limit type made precise with MaxL/MinL [v4]

[Emanuel Peter]

On Sat, 15 Apr 2023 06:35:02 GMT, Jasmine Karthikeyan <jkarthikeyan at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Review suggestion by Tobias Hartmann
>>   
>>   Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>
>
> I only had a handful of cases so I've attached them in [this gist](https://gist.github.com/jaskarth/878648eecaf74e168d5499c5961b4715).
> I found these a few weeks ago, but looking back I think I have misremembered the problem chain. Examples 3-5 may be a different bug as they deal with `ConvL2I->ConvI2L` chains instead of `ConvI2L->ConvL2I` chains as you are seeing, as the latter has an Identity() transform defined while it seems the former does not- I apologize for the noise if the issues are unrelated. Examples 1 and 2 could perhaps still be useful in diagnosing the issue, as they describe cases where ideal transforms that do exist aren't taken. I tried looking into that bug myself a while ago but didn't get far.
> 
> JDK-8298951 is exciting, it'll make reasoning about middle-end optimizations a lot easier :)

@jaskarth I think your issues are not related, though I can look at them again once I get back to IGVN verification.

@vnkozlov I thought about it a bit more. With a simple example like `Test::test`, I get unrolling `2048`, so we unroll 10-ish times. I see accordingly many `ConvI2L, MaxL, ConvL2I` nodes. Now, I can collapse the `ConvL2I -> ConvI2L` parts (the types guarantee that we never leave the `int` range, so conversion never clips anything), so it is only a chain of `MaxL` nodes. The issue at that point: I now basically have a reduction of all the "limits" I have ever wanted to respect (including some range-check limits, and all the unroll limits: `limit, limit-1, limit-3, limit-7, limit-15, limit-31, ... limit-1023`. The problem is that most of them have quite large ranges that are overlapping, so we cannot fold them at that point.

The example:

`./java -Xcomp -XX:CompileCommand=compileonly,Test::test -XX:CompileCommand=printcompilation,Test::test -XX:+TraceLoopOpts -XX:+PrintIdeal Test.java`


public class Test {
    static int START = 0;
    static int FINISH = 512;
    static int RANGE = 512;

    public static void main(String args[]) {
        byte[] data = new byte[RANGE];
        test(data);
    }

    public static void test(byte[] data) {
        for (int j = START; j < FINISH; j++) {
            data[j] = (byte)(data[j] * 11);
        }
    }
}


What to do with this?
 - Performance testing did not show any difference. But maybe we do not trust that enough.
 - Before and now, the chain of unrolling-limits can be interrupted by range-check limits. We probably will just accept that this means that not all of the unrolling-limits can be folded together.

**I have an alternative proposal:**
Leave the `MaxL/MinL` node for the range-check limits, there are usually not that many RC-limits, and up to now we used a `CMove` node per such limit already anyway.

But for the unroll-limits, we introduce a `SubINoUnderflow` node, which does a safe (no-underflow) subtraction `limit-stride`.
These nodes can be folded together relatively easily.
I already had such an implementation before, and reverted it https://github.com/openjdk/jdk/pull/13269/commits/f5fcf6084a2446876ba2a85907a2991ef4c705b7
I had already discussed this idea with @chhagedorn a while ago. But then decided against it once I also saw that I wanted a unified solution for RC-limits and unroll-limits. But now I'm questioning that decision again.

With this `SubINoUnderflowNode` idea, we would have a constant number of nodes added per RC-limit. And then for all the unroll limit adjustments together, we would only have one `SubINoUnderflow` node, as they would all collapse into one. At macro expansion, I can then expand it into a single CMove node.

@vnkozlov What do you think? Do you have any other ideas?

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

PR Comment: https://git.openjdk.org/jdk/pull/13269#issuecomment-1511456666