RFR: 8327381 Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value [v2]

[Jasmine Karthikeyan]

On Mon, 11 Mar 2024 16:52:07 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

>> This PR resolves [JDK-8327381](https://bugs.openjdk.org/browse/JDK-8327381)
>> 
>> Currently the transformations for expressions with patterns `((x & m) u<= m)` or `((m & x) u<= m)` to `true` is in `BoolNode::Ideal` function with a new constant node of value `1` created. However, this is technically a type-improving (reduction in range) transformation that's better suited in `BoolNode::Value` function.
>> 
>> New unit test `test/hotspot/jtreg/compiler/c2/TestBoolNodeGvn.java` asserting on IR nodes and correctness of this transformation is added and passing.
>
> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix test by adding the missing inversion
>   
>   also excluding negative values for unsigned comparison

I think the cleanup looks good! I have mostly stylistic suggestions here. Also, the copyright header in `subnode.cpp` should be updated to read 2024.

src/hotspot/share/opto/subnode.cpp line 1808:

> 1806: // based on local information.   If the input is constant, do it.
> 1807: const Type* BoolNode::Value(PhaseGVN* phase) const {
> 1808:   Node *cmp = in(1);

It's preferred to use `Type*` for pointer types, so this `Node *var` (and the others below) should be `Node* var`.

src/hotspot/share/opto/subnode.cpp line 1809:

> 1807: const Type* BoolNode::Value(PhaseGVN* phase) const {
> 1808:   Node *cmp = in(1);
> 1809:   if (cmp && cmp->is_Sub()) {

Suggestion:

  if (cmp != nullptr && cmp->is_Sub()) {

The `cmp` condition should be `cmp != nullptr`, to make it more clear what is being compared.

test/hotspot/jtreg/compiler/c2/TestBoolNodeGvn.java line 38:

> 36:  * @summary
> 37:  * @library /test/lib /
> 38:  * @run driver compiler.c2.TestBoolNodeGvn

I think it'd be better to move the test to `c2.irTests`, so that it's grouped with other IR tests. Also, it would be good to add a `@bug` tag and fill out the `@summary` tag.

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

PR Review: https://git.openjdk.org/jdk/pull/18198#pullrequestreview-1929183892
PR Review Comment: https://git.openjdk.org/jdk/pull/18198#discussion_r1520397799
PR Review Comment: https://git.openjdk.org/jdk/pull/18198#discussion_r1520394190
PR Review Comment: https://git.openjdk.org/jdk/pull/18198#discussion_r1520411065