RFR: 8353359: C2: Or(I|L)Node::Ideal is missing AddNode::Ideal call

[Emanuel Peter]

On Thu, 3 Apr 2025 07:29:50 GMT, Hannes Greule <hgreule at openjdk.org> wrote:

>> @SirYwell Wow, good find! Oh dear, things like this are so easy to get wrong. Thanks for writing the IR test, that seems really to be the only way to ensure we don't get these kinds of regressions. I wonder how many more of these kinds of issues we have... Optimal would be if we had IR tests for every optimization, but that would be a lot of work!
>> 
>> I'm running some testing, please ping me in 24h for the results!
>
>> @SirYwell Wow, good find! Oh dear, things like this are so easy to get wrong. Thanks for writing the IR test, that seems really to be the only way to ensure we don't get these kinds of regressions. I wonder how many more of these kinds of issues we have... Optimal would be if we had IR tests for every optimization, but that would be a lot of work!
>> 
>> I'm running some testing, please ping me in 24h for the results!
> 
> Thanks @eme64, did the test go through?
> 
> I'm wondering now if there should rather be something like an `AddNodeIdealizationTests.java` that contains the optimizations of AddNode::ideal for all(*) its subtypes rather than more specific test classes testing a mix of optimizations from different Ideal methods (e.g., `AddINodeIdealizationTests.java` has a test for `(x + 1) + 2 => x + 3`). I'm not sure if your current work on the template library could somehow cover that (replacing operators, replacing IR check rules).
> (*) Not all, as there are some subtypes for which the optimizations don't apply.
> 
> I also noticed that I didn't add the bug id to the test headers here, I'll add them before merging.

@SirYwell Something like a `AddINodeIdealizationTests.java` sounds like a good idea. We could systematically cover `Value`, `Ideal` and `Identity`, for every single node. A good structure would really help. But collecting / writing all those IR tests is a lot of work... But we could at least start setting it up, and extend it over time. But that is work for some separate RFE's, I'll discuss it with my co-workers.

Not sure if Templates really help. Because the tedious work is capturing all the patterns, and writing IR rules. That's hard to automate I think. But maybe you have some good ideas here :)
Well, I suppose some patterns go over multiple types, so there we could do something. And maybe we can still cut a lot of boiler-plate code with Templates, and get a better overview that way... worth thinking about a little more!

Tests have passed. I'll wait with approval until you make the updates :)

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

PR Comment: https://git.openjdk.org/jdk/pull/24348#issuecomment-2774753603