On Thu, 1 May 2025 07:32:22 GMT, erifan <d...@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
