RFR: 8353551: C2: Constant folding for ReverseBytes nodes [v2]

[Hannes Greule]

On Tue, 8 Apr 2025 19:56:14 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> @j3graham I mainly want to avoid conflicts or duplicated solutions. I meant that after #17508 this code can be further generalized anyway, allowing to use the `try_cast` function then. This can obviously happen in a separate PR, if this one is integrated before.
>> 
>> @iwanowww I'm not sure if that's possible without more duplication. We need to choose the correct byteswap implementation depending on the node's type. Please let me know if I'm missing something.
>
> It does have some duplication, but simply in a different place. I wouldn't say there's more of it:
> 
> static const Type* reverse_bytes(int op, const Type* con) {
>   switch (op) {
>     case Op_ReverseBytesS:  return TypeInt::make(byteswap(checked_cast<jshort>(con->is_int()->get_con())));
>     case Op_ReverseBytesUS: return TypeInt::make(byteswap(checked_cast<jchar> (con->is_int()->get_con())));
>     case Op_ReverseBytesI:  return TypeInt::make(byteswap(con->is_int()->get_con()));
>     case Op_ReverseBytesL:  return TypeLong::make(byteswap(con->is_long()->get_con()));
> 
>     default: ShouldNotReachHere();
>   }
> }
> 
> const Type* ReverseBytesNode::Value(PhaseGVN* phase) const {
>   const Type* type = phase->type(in(1));
>   if (type == Type::TOP) {
>     return Type::TOP;
>   }
>   if (type->singleton()) {
>     return reverse_bytes(Opcode(), type);
>   }
>   return bottom_type();
> }
> 
> 
> At some point, we should consider folding `ReverseBytes*Node` specializations into a single one (`ReverseBytesNode`) parameterized by element type (as was done for `ReverseBytesV` and other vector nodes). After it is done, there won't be a convenient way to piggyback on virtual calls to hook specialized versions forcing us to do explicit dispatch anyway.

Okay, I see. That works for me. One problem though, there currently isn't a common `ReverseBytesNode` type, so I'd need to add that. And I assume that should be a `TypeNode` then? In that case, as `ReverseBytes*Node`s are `InvolutionNode`s, would `InvolutionNode` need to be a `TypeNode`? Or can I use multiple inheritance? (I didn't see any example of that in current Node types). Both are doable, and the first might even make sense, but I'm not sure if it's a bit much for this PR.

Or do you want me to (temporarily) duplicate the Value code (i.e. move more code from the templated function to the Value functions, and only keep the simple `reverse_bytes` from your snippet)?

Alternatively, I'd also be fine to put this PR on hold and make InvolutionNode a TypeNode first, or whatever you think is best. Please let me know what you think.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24382#discussion_r2034940786