RFR: 8353551: C2: Constant folding for ReverseBytes nodes [v2]
Vladimir Ivanov
vlivanov at openjdk.org
Wed Apr 9 17:58:44 UTC 2025
On Wed, 9 Apr 2025 17:53:48 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:
>> 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.
>
>> And I assume that should be a TypeNode then?
>
> Why do you think it benefits from becoming a `TypeNode`?
>
>> In that case, as ReverseBytes*Nodes are InvolutionNodes, would InvolutionNode need to be a TypeNode? Or can I use multiple inheritance?
>
> We try to avoid multiple inheritance in JVM and C2 doesn't use any AFAIK.
>
> Actually, I had an afterthought about `InvolutionNode` after approving it. It looks a bit weird to model "involution" property through inheritance. (Primarily, because it's hard to mix multiple properties.) Node flags would be a better fit IMO.
For now, I suggest to just add a superclass`ReverseBytesNode` which extends `InvolutionNode` and place `Value` there.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24382#discussion_r2035862453
More information about the hotspot-compiler-dev
mailing list