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

[Emanuel Peter]

On Fri, 30 Jun 2023 08:01:29 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>> 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`).
> 
> Thanks a lot!
> 
>> 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?
> 
> We (Arm) will do the real fix. `UseSVE=0` is needed when `sve, true`, which only affects this test now. And yes, I can revert these IR checks once the real fix is in. 
> 
>> 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.
> 
> Agree, although we only use `UseSVE` in backend now.
> 
>> 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?
> 
> I prefer fixing that in a separate patch. One reason is syncing the vm options and cpu features is a refactory to AArch64 backend for me. It has other relative cpu features specific to different SVE systems besides `sve`. For example, the `svebitperm` which exists after sve2.  We have to take a consideration for them as well. Besides, although the changes is not so big, we have to do more testing to make sure no regressions are involved. 
> 
> And besides the `UseSVE`, do you think it's necessary to sync other options as well?
> 
>> Anyway, I just launched testing for commit 1: tier1-6 plus stress testing. Will report back on Monday probably.
> 
> Thanks for doing this!

@XiaohongGong Testing for commit 1 looks good.

I don't have the expertise on the other CPU features. But from a distance, I'd say yes. The idea with all the CPU features is that we can simulate less powerful machines with more powerful machines. But I would first fix the SVE features, and handle the others separately. Maybe for those a separate discussion needs to happen first.
@vnkozlov What do you think about syncing CPU features with their VM Flags? Only relevant for `AVX` and `SVE`, or more generally? What about `UseSHA`, `UseAES` for example?

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

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