RFR: 8256614: AArch64: Add SVE backend implementation for integer min/max

[Andrew Dinn]

On Mon, 23 Nov 2020 04:56:32 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>>> > We need to add them all to avoid the "bad AD file" crash.
>>> 
>>> I'm not clear how this came about. Is this fix needed as a response to another change that caused the crash? Or is the problem that the incomplete implementation was pushed without testing some cases properly?
>>> 
>>> Is there a before and after test case for the problem this fixes?
>> 
>> Hi @adinn , thanks for looking at this PR. Currently only the VectorAPI can generate the `MinVNode/MaxVNode` for integer types. I found this issue when I tried to use the related API to do some other investigation work. And actually there is the jtreg tests (eg: https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/incubator/vector/IntMaxVectorTests.java#L2403) for it. I think the reason that we didn't find the issue is the loop count `INVOC_COUNT` , which is too small that didn't make the method hot enough to be compiled with C2. Currently the value is set to 100:
>> static final int INVOC_COUNT = Integer.getInteger("jdk.incubator.vector.test.loop-iterations", 100);
>> When I use a larger loop count like 10000, the test can crash with `bad AD file`.  So this issue can be reproduced by adding `-Djdk.incubator.vector.test.loop-iterations=10000` when running the tests.
>
>> Hi @XiaohongGong . Thanks for clarifying that. Your code changes look ok to me but it would be good if we could also change the default setting for this loop count to ensure that this case gets tested when SVE hw is present.
>> 
>> iterations=100 appears to be defaulted in file config.sh in the same dir as the test program. Do you know if using iterations=100 is actually triggering C2 compilation for any case (SVE or other, including on Intel)? If not then we really need to increase the default count to a higher value for all cases.
>> 
>> If this is just an SVE-specific thing then we could maybe add some special case processing to the config script to detect an arch where SVE is present and set a higher value.
> 
> Yeah, I agree that it's better to adjust the loop count for jtreg tests if necessary. Do you think it's better to change it with another patch since I think it's not a SVE special case (NEON and X86 have the same issue)? Changing the default loop count for all tests needs more time to run the tests and make the tests easier to time out. And  I'v no idea about how to set the suitable value to balance the time and effectiveness.  I think some cases can actually trigger the C2 compilation if using default iterations=100. However, it cannot for simple cases like `min/max`.

>Yeah, I agree that it's better to adjust the loop count for jtreg tests if necessary. Do you think it's better to change it with another patch since I think it's not a SVE special case (NEON and X86 have the same issue)? Changing the default loop count for all tests needs more time to run the tests and make the tests easier to time out. And I've no idea about how to set the suitable value to balance the time and effectiveness. I think some cases can actually trigger the C2 compilation if using default iterations=100. However, it cannot for simple cases like min/max.

Yes, please raise a separate patch for the iteration count change. We can discuss the timing vs time-out issue there, including the option of splitting out simple tests that need higher counts. If you can do that then I'm happy with this patch as it is.

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

PR: https://git.openjdk.java.net/jdk/pull/1337