RFR: 8313672: C2: PhaseCCP does not correctly track analysis dependencies involving shift, convert, and mask [v8]

[Emanuel Peter]

On Fri, 3 Nov 2023 12:34:05 GMT, Daniel Lundén <duke at openjdk.org> wrote:

>> src/hotspot/share/opto/node.hpp line 1744:
>> 
>>> 1742:         internals.push(internal_use); // traverse this also
>>> 1743:       } else {
>>> 1744:         callback(internal_use);
>> 
>> uses may now have the callback invoked multiple times, for example if we have something like this:
>> 
>> 
>>             this
>>              |
>>    +---------+--------+
>>    |                  |
>>    x                  y
>>    |                  |
>>    +--------+ +-------+
>>             | |
>>             use
>> 
>> This could not happen with Cast nodes (I think), but now that we generalize the function we should improve the logic, such that callback only gets called once.
>
> Yes, it is true that we sometimes may visit nodes multiple times. However, is it worth the extra bookkeeping overhead to ensure uses are visited only once? At least for the current applications of `visit_uses`, it's not unsound to (quite rarely, I think) add a node twice or more times to worklists.
> 
> I'm fine with either option, let me know what you think.

I agree, it will be rare. But we don't know what it will be used for. One might want to count all uses or whatever, and then duplicates could matter.

You can also leave a clear comment, that uses may be called multiple times, then it is up to the user.

I would simply have renamed `internals` -> `worklist`.
And then push all nodes to it.
But when you look at a node, you first check it `is_boundary` -> `callback`.
Else put all its outputs on `worklist`.
I think the overhead is not that much.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16429#discussion_r1383382818