RFR: 8364407: [REDO] Consolidate Identity of self-inverse operations
Jasmine Karthikeyan
jkarthikeyan at openjdk.org
Mon Aug 18 17:52:18 UTC 2025
On Mon, 18 Aug 2025 15:49:48 GMT, Hannes Greule <hgreule at openjdk.org> wrote:
> The previous approach was flawed for `short` and `char` as these are int-subtypes and truncate the result (see the backout issue https://bugs.openjdk.org/browse/JDK-8364409 for a reproducer).
>
> This change now first ensures that the input type is small enough so no truncation gets lost when dropping the operations.
>
> The previous implementation also used an `InvolutionNode` superclass with one `Identity(...)` implementation, but there were some reservations whether this is the right way to go. As we now have a `ReverseBytesNode`, there is also less benefit in having the supertype, as this covers 4 in 1 already.
>
> I also added test cases on top of the original ones that ensure the nodes stay when we can't prove the input type is small enough.
This looks nice! I've just left some style comments.
src/hotspot/share/opto/subnode.cpp line 2075:
> 2073: if (involution->in(1)->Opcode() == involution->Opcode()) {
> 2074: Node* original = involution->in(1)->in(1);
> 2075: const TypeInt *type = phase->type(original)->isa_int();
Suggestion:
const TypeInt* type = phase->type(original)->isa_int();
src/hotspot/share/opto/subnode.cpp line 2076:
> 2074: Node* original = involution->in(1)->in(1);
> 2075: const TypeInt *type = phase->type(original)->isa_int();
> 2076: // Operations on sub-int types might not be "real" involutions for values outside their type range.
I think it would be helpful to state an example of the disallowed case in the comment, maybe something like:
Suggestion:
// Operations on sub-int types might not be "real" involutions for values outside their type range, for example a ReverseBytesS node with an input larger than short.
test/hotspot/jtreg/compiler/c2/gvn/InvolutionIdentityTests.java line 132:
> 130: assertResultD(nand);
> 131:
> 132: }
There's an extra whitespace:
Suggestion:
}
-------------
PR Review: https://git.openjdk.org/jdk/pull/26823#pullrequestreview-3129186828
PR Review Comment: https://git.openjdk.org/jdk/pull/26823#discussion_r2282955571
PR Review Comment: https://git.openjdk.org/jdk/pull/26823#discussion_r2283065869
PR Review Comment: https://git.openjdk.org/jdk/pull/26823#discussion_r2282982876
More information about the hotspot-compiler-dev
mailing list