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

[Christian Hagedorn]

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

>> Hi,
>> 
>> this simple change adds a missing AddNode::Ideal call to Or(I|L)Node::Ideal. See the added tests for examples of optimizations that don't apply without this change.
>> 
>> Please let me know what you think.
>
> Hannes Greule has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - update license year
>  - add bug id

Indeed, good catch! Very hard to find these bugs but I'm afraid there is, unfortunately, not much we can do about it instead of having more IR tests to catch these cases.

> 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 would recommend to split it up more to easier find the tests again. I would probably first search for a `Or*Tests.java` instead of looking into `Add*Tests.java` when checking tests for `OrINode`. We can still group together multiple nodes if they only differ in the basic types like `AddI` and `AddL`. But this can still be discussed when such tests are added, which I totally agree with you we should have. Adding tests can be done incrementally and even in small or "not yet completely covering a node with many transformation" batches. 

> We could systematically cover Value, Ideal and Identity, for every single node

That would be great!

>  Because the tedious work is capturing all the patterns, and writing IR rules.

What might help here is when the  documentation for the `Ideal()`, `Identity()`, and `Value()` methods would enumerate the different optimizations which we want to check and add the numbers to the method body where it's implemented. That would not only help to map documentation to code and spot potentially missing/wrong promises but also helps with writing clearly map-able tests: We can simply write `testAddINodeCase3b()`, for example instead of `testAddISomeHardToMapOptimizationName()`. The only downside of that: when we change the enumerations, the tests are no longer in sync. But I guess that's an okay price to pay.

> But that is work for some separate RFE's

Definitely!

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24348#pullrequestreview-2739029429