On Tue, 29 Apr 2025 10:22:22 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>> erifan has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains four additional commits since >> the last revision: >> >> - Addressed some review comments >> >> 1. Call VectorNode::Ideal() only once in XorVNode::Ideal. >> 2. Improve code comments. >> - Merge branch 'master' into JDK-8354242 >> - Merge branch 'master' into JDK-8354242 >> - 8354242: VectorAPI: combine vector not operation with compare >> >> This patch optimizes the following patterns: >> For integer types: >> ``` >> (XorV (VectorMaskCmp src1 src2 cond) (Replicate -1)) >> => (VectorMaskCmp src1 src2 ncond) >> (XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1)) >> => (VectorMaskCmp src1 src2 ncond) >> ``` >> cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the >> negative comparison of cond. >> >> For float and double types: >> ``` >> (XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1)) >> => (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) >> (XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1)) >> => (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) >> ``` >> cond can be eq or ne. >> >> Benchmarks on Nvidia Grace machine with 128-bit SVE2: >> With option `-XX:UseSVE=2`: >> ``` >> Benchmark Unit Before Score Error After >> Score Error Uplift >> testCompareEQMaskNotByte ops/s 7912127.225 2677.289518 >> 10266136.26 8955.008548 1.29 >> testCompareEQMaskNotDouble ops/s 884737.6799 446.963779 >> 1179760.772 448.031844 1.33 >> testCompareEQMaskNotFloat ops/s 1765045.787 682.332214 >> 2359520.803 896.305743 1.33 >> testCompareEQMaskNotInt ops/s 1787221.411 977.743935 >> 2353952.519 960.069976 1.31 >> testCompareEQMaskNotLong ops/s 895297.1974 673.44808 >> 1178449.02 323.804205 1.31 >> testCompareEQMaskNotShort ops/s 3339987.002 3415.2226 >> 4712761.965 2110.862053 1.41 >> testCompareGEMaskNotByte ops/s 7907615.16 4094.243652 >> 10251646.9 9486.699831 1.29 >> testCompareGEMaskNotInt ops/s 1683738.958 4233.813092 >> 2352855.205 1251.952546 1.39 >> testCompareGEMaskNotLong ops/s 854496.1561 8594.598885 >> 1177811.493 521.1229 1.37 >> testCompareGEMaskNotShort ops/s 3341860.309 1578.975338 >> 4714008.434 1681.10365 1.41 >> testCompareGTMaskNotByte ops/s 7910823.674 2993.367032 >> 10245063.58 9774.75138 1.29 >> testCompareGTMaskNotInt ops/s 1673393... > > Yes, this discussion is down to `requires` vs `applyIf`. This is my argument > for `applyIf`, quoted from above, I have not yet seen an argument against it: > >> If you use @require, then the person does not realize there is a test AND >> the test is not run. If you use applyIf, the person does not realize there >> is a test, but it is run at least for result verifiation - and then the >> person MIGHT realize if the test catches a wrong result / crash. > > In my understanding, `requires` should only be used if the test really > **requires** a certain platform or feature. That can be because some flags > are only available under certain platforms for example. But for IR tests, we > should try to always use `applyIf`, because it allows testing on other > platforms. > > Actually, I filed this RFE a while ago: > https://bugs.openjdk.org/browse/JDK-8310891 > We should try to move as many tests from using `requires` to `applyIf`, so > that we have an increased test coverage. @eme64 @jatin-bhateja I have updated the test, thanks for your suggestion. ------------- PR Comment: https://git.openjdk.org/jdk/pull/24674#issuecomment-2844256626
