RFR: 8309894: compiler/vectorapi/VectorLogicalOpIdentityTest.java fails on SVE system with UseSVE=0

[Christian Hagedorn]

On Mon, 19 Jun 2023 01:49:57 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

> This test fails with several IR check failures when run on ARM SVE systems with vm option `-XX:UseSVE=0`. Here is one of the failed IR rule:
> 
> 
> @IR(counts = {IRNode.AND_V, "1"}, applyIfCPUFeatureOr = {"sve", "true", "avx512", "true"})
> public static void testAndMaskSameValue1()
> 
> The specified IR in the test depends on the platform's predicate feature. Hence the IR check can be applied only on ARM SVE or X86 AVX512 systems. But with `-XX:UseSVE=0` on ARM SVE machines, JVM will disable SVE feature for compiler. But the CPU feature is not changed. To guarantee the IR rule is run with SVE as expected, it has to add another condition like `applyIf = {"UseSVE", ">0"}`. Consider `UseSVE` is an ARM specific option, it can be used only on ARM CPUs. So the right IR rules should be:
> 
> 
> @IR(counts = {IRNode.AND_V, "1"}, applyIfCPUFeature = {"sve", "true"}, applyIf = {"UseSVE", "> 0"})
> @IR(counts = {IRNode.AND_V, "1"}, applyIfCPUFeature = {"avx512", "true"})
> public static void testAndMaskSameValue1()
> 
> 
> This patch also changes the check order of conditions for a IR rule. It's better to check `applyIfCPUFeature` before `applyIf`, in case the vm option is invalid on running hardware, which makes test throw exception and abort.
> 
> Verified on X86 systems with `UseAVX=1/2/3` by removing the test from ProblemList.txt, and SVE systems with `UseSVE=0/1`.

> Accepted as a temporary fix that has to be reverted with [JDK-8311130](https://bugs.openjdk.org/browse/JDK-8311130).

I agree with this temporary fix and reverting it again when syncing the flags with the CPU features in JDK-8311130.

test/hotspot/jtreg/compiler/lib/ir_framework/README.md line 99:

> 97: Sometimes, an `@IR` rule should only be applied if a certain CPU feature is present. This can be done with the attributes `applyIfCPUFeatureXXX` in [@IR](./IR.java) which follow the same logic as the `applyIfXXX` methods for flags in the previous section. An example with `applyIfCPUFeatureXXX` can be found in [TestCPUFeatureCheck](../../../testlibrary_tests/ir_framework/tests/TestCPUFeatureCheck.java) (internal framework test).
> 98: 
> 99: If a `@Test` annotated method has multiple preconditions (for example `applyIf` and `applyIfCPUFeature`), they are evaluated as a logical conjunction. It's worth noting that flags in `applyIf` are checked only if the cpu features in `applyIfCPUFeature` are matched when they are both specified. This can avoid the vm option being evaluated on hardware that does not support it. An example with both `applyIfCPUFeatureXXX` and `applyIfXXX` can be found in [TestPreconditions](../../../testlibrary_tests/ir_framework/tests/TestPreconditions.java) (internal framework test).

Suggestion:

If a `@Test` annotated method has multiple preconditions (for example `applyIf` and `applyIfCPUFeature`), they are evaluated as a logical conjunction. It's worth noting that flags in `applyIf` are checked only if the CPU features in `applyIfCPUFeature` are matched when they are both specified. This avoids the VM flag being evaluated on hardware that does not support it. An example with both `applyIfCPUFeatureXXX` and `applyIfXXX` can be found in [TestPreconditions](../../../testlibrary_tests/ir_framework/tests/TestPreconditions.java) (internal framework test).

test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestPreconditions.java line 65:

> 63:     // Note that precondition `applyIfCPUFeature` will be evaluated first with
> 64:     // early return. Hence the IR check should not be applied on non-aarch64
> 65:     // systems, and no exception happens.

Maybe you can expand here that no exception is thrown because we are not checking the value of the unsupported `UseSVE` flag on non-aarch64 systems. Same for x86 and `UseAVX` below.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14533#pullrequestreview-1506628333
PR Review Comment: https://git.openjdk.org/jdk/pull/14533#discussion_r1250513134
PR Review Comment: https://git.openjdk.org/jdk/pull/14533#discussion_r1247511655