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

[Emanuel Peter]

On Fri, 30 Jun 2023 02:30:20 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`.
>
> Hi there, I'v filed the vm option and cpu feature sync issue here for AArch64: https://bugs.openjdk.org/browse/JDK-8311130, and will address the comment with it. Thanks again for the advice!
> 
> Hi @eme64 , besides the sync issue, does the change to IR framework make sense to you? Currently, if we use an architecture specific vm options with `applyIf` for an IR check, and run the test on another different architecture, the whole test will fail by throwing exceptions, even if we add the `applyIfCPUFeature` to do the cpu check. The changes in the IR framework can fix this issue. 
> 
> If that part seems fine to you, maybe we can let this PR in first? Since the test failure will noise our internal ci testing. WDYT? Thanks!

@XiaohongGong I totally agree with the changes to the IR framework (having `applyIfCPUFeature` before `applyIf`).

Otherwise, using both `UseSVE=0` and `sve, false` is a temporary fix that should be reverted after [JDK-8311130](https://bugs.openjdk.org/browse/JDK-8311130). I'm accepting it as a temporary fix only. Who will do the real fix?

I was a bit afraid not keeping the CPU feature and the VM flag in sync could also lead to issues in the backend of aarch64. But it does indeed seem that we only use `UseSVE`,  and never `VM_Version::supports_sve()`. Still, someone might use them synonymous in the future and expect that they are in sync.

Actually, since there are only so few uses of `VM_Version::supports_sve()`, is the risk not very low to just mask off the feature now directly with this fix? That fix does not look so complicated as I feared. What do you think?

Anyway, I just launched testing for commit 1: tier1-6 plus stress testing. Will report back on Monday probably.

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

PR Comment: https://git.openjdk.org/jdk/pull/14533#issuecomment-1614251920