On Mon, 28 Apr 2025 14:10:40 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>> test/hotspot/jtreg/compiler/vectorapi/VectorMaskCompareNotTest.java line 237:
>>
>>> 235: public static void testCompareEQMaskNotByte() {
>>> 236: testCompareMaskNotByte(VectorOperators.EQ);
>>> 237: }
>>
>> Another comment: you now only have "negative" tests, where you check for
>> count `=0`. It would be good if you also had a positive rule here, one where
>> you do see an XOR in a similar case, where your optimization does apply.
>>
>> This would basically be a "control" that checks that your are testing the
>> right thing.
>
> Also: this test should vectorize on some plarforms, right? A compare,
> correct? Would it not be good to actively check that with an IR rule?
> Another comment: you now only have "negative" tests, where you check for
> count =0. It would be good if you also had a positive rule here, one where
> you do see an XOR in a similar case, where your optimization does apply.
This would basically be a "control" that checks that your are testing the right
thing.
This is not a negative test, this is a positive test. What this patch does is:
`Vector compare(NE) + not() => vector compare(EQ)`. The `not()` operation is
removed. For details, please see the commit message and related code. So here
we check XorV and XorVMask == 0, which I think is reasonable.
For negative tests, I think it's not necessary, because I only test what I
optimized, and I won't write a test to say what optimization cannot be done.
> Also: this test should vectorize on some plarforms, right? A compare,
> correct? Would it not be good to actively check that with an IR rule?
The `compare` operation is not eliminated, the patch aims to eliminate the
`not` operation following `compare`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24674#discussion_r2065195738
