RFR: 8354242: VectorAPI: combine vector not operation with compare [v3]

[erifan]

On Thu, 1 May 2025 07:32:22 GMT, erifan <duke at openjdk.org> wrote:

>> 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.

> @erifan thanks for updating the tests!
> 
> Now I had a quick look at the VM code.
> 
> My biggest observation is this:
> 
> Wrapping `VectorNode::Ideal` somewhere in the middle of your new optimization is going to make future optimizations here much harder. How would they check their conditions next to yours? That would be quite a mess.
> 
> I suggest you do this:
> 
> * `XorVNode::Ideal` does
>   
>   * checks `in1 == in2` case
>   * calls a method called `XorVNode::Ideal_XorV_VectorMaskCmp`. Check if it succeeded, i.e. returns `nullptr`.
>   * ... future optimizations could go here ...
>   * Finally, i.e. none of the optimizations above worked: call `VectorNode::Ideal`
> 
> Then you pack all your new logic here into `XorVNode::Ideal_XorV_VectorMaskCmp`. You can also find a better name, it is just what I came up with just now.
> 
> This gives us a much more **modular** design, and it is easier to add another new optimization to `XorVNode::Ideal`. It is easy to change the precedence of the optimizations by just changing the order, etc.
> 
> Examples of this "modular" design:
> 
> * `CMoveNode::Ideal` -> calls `TypeNode::Ideal` and `Ideal_minmax`.
> * `StoreBNode::Ideal` -> calls `StoreNode::Ideal_masked_input` and `StoreNode::Ideal_sign_extended_input`
>   These are really nice, because you can quickly see what optimizations we already have, and in which order they are checked.

Yes, this is a good idea, I have changed it like this, thanks for your suggestion.

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

PR Comment: https://git.openjdk.org/jdk/pull/24674#issuecomment-2856811448