RFR: 8347459: C2: missing transformation for chain of shifts/multiplications by constants [v4]

[Jasmine Karthikeyan]

On Tue, 25 Feb 2025 13:05:13 GMT, Marc Chevalier <duke at openjdk.org> wrote:

>> src/hotspot/share/opto/mulnode.cpp line 996:
>> 
>>> 994: 
>>> 995:   if (con0 + con1 >= nbits) {
>>> 996:     return ConNode::make(TypeInteger::zero(bt));
>> 
>> It'd be clearer to do this, which is more equivalent but more concise:
>> Suggestion:
>> 
>>     return phase->zerocon(bt);
>
> Actually, this is not equivalent and incorrect. I've did this exact mistake in an earlier version. The problem is that `zerocon` caches the nodes:
> https://github.com/openjdk/jdk/blob/8cfebc41dc8ec7b0d24d9c467b91de82d28b73fc/src/hotspot/share/opto/phaseX.cpp#L654-L656
> So, then, we likely (or at least may) return an old node, which is not legal: `Ideal` is only allowed to return `this`, `nullptr` or a new node. But yes, it's unfortunate because it'd be much lighter to read.

Ah, that's tricky! But that makes a lot of sense to me. Maybe it could be worth adding a comment there to mention the fact.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23728#discussion_r1971822108