RFR: 8359602: Ideal optimizations depending on input type are missed because of missing notification mechanism from CCP [v2]

[Benoît Maillard]

On Tue, 1 Jul 2025 07:07:40 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

>> @benoitmaillard Very nice work, and great description :)
>> 
>>>Did you check if this allows enabling any of the other disabled verifications from [JDK-8347273](https://bugs.openjdk.org/browse/JDK-8347273)?
>> 
>> That may be a lot of work. Not sure if it is worth checking all of them now. @TobiHartmann how much should he invest in this now? An alternative is just tackling all the other cases later. What do you think?
>> 
>> @benoitmaillard One more open question for me: `raise_bottom_type` only sets the node internal `_type`. But in IGVN, we do not read from `_type` but `phase->type(in(2))`. Do you know when the `phase->type(in(2))` value changes? Is that also during CCP? Before or after the `_type` is modified?
>
>> @benoitmaillard Very nice work, and great description :)
> 
> Thank you! @eme64 
> 
>> > Did you check if this allows enabling any of the other disabled verifications from [JDK-8347273](https://bugs.openjdk.org/browse/JDK-8347273)?
>> 
>> That may be a lot of work. Not sure if it is worth checking all of them now. @TobiHartmann how much should he invest in this now? An alternative is just tackling all the other cases later. What do you think?
> 
> I have started to take a look at this and it seems that there are a lot of cases to check indeed.
> 
>> @benoitmaillard One more open question for me: `raise_bottom_type` only sets the node internal `_type`. But in IGVN, we do not read from `_type` but `phase->type(in(2))`. Do you know when the `phase->type(in(2))` value changes? Is that also during CCP? Before or after the `_type` is modified?
> 
> Yes, good point, I should I have mentioned this somewhere. The `phase->type(in(2))` call uses the type array from `PhaseValues`. The type array entry is actually modified earlier, in `PhaseCCP::analyze`, right after the `Value` call. You can see the `set_type` call [here](https://github.com/benoitmaillard/jdk/blob/75de51dff6d9cc3e9764737b29b9358992b488b7/src/hotspot/share/opto/phaseX.cpp#L2765). When this happens, users are added to the (local) worklist but again it does not change our issue as only value optimizations occur in that context.

> > > @benoitmaillard One more open question for me: `raise_bottom_type` only sets the node internal `_type`. But in IGVN, we do not read from `_type` but `phase->type(in(2))`. Do you know when the `phase->type(in(2))` value changes? Is that also during CCP? Before or after the `_type` is modified?
> > 
> > 
> > Yes, good point, I should I have mentioned this somewhere. The `phase->type(in(2))` call uses the type array from `PhaseValues`. The type array entry is actually modified earlier, in `PhaseCCP::analyze`, right after the `Value` call. You can see the `set_type` call [here](https://github.com/benoitmaillard/jdk/blob/75de51dff6d9cc3e9764737b29b9358992b488b7/src/hotspot/share/opto/phaseX.cpp#L2765). When this happens, users are added to the (local) worklist but again it does not change our issue as only value optimizations occur in that context.
> 
> Thanks for the explanation! So it seems that `CCP` and `IGVN` share the type array, right? Ah yes, it is the `Compile::_types`:
> 
> ```
>    461   // Shared type array for GVN, IGVN and CCP. It maps node idx -> Type*. 
>    462   Type_Array*           _types;
> ```
> 
> If the value behind `phase->type(in(2))` (the type array entry) is modified in `PhaseCCP::analyze`, right after the `Value` call, then why not do the notification there? If we did that, we would do more notification than what you now proposed (to do the notification in `PhaseCCP::transform_once` on the nodes that have a type that is different than the `bottom_type`). Are we possibly missing any important case with your approach now? Probably not, I would argue: with your approach we still notify for all live nodes that have a modified type, or are replaced with a constant. If we notified after every type update in `PhaseCCP::analyze`, we might notify for nodes multiple times, and we would also notify for nodes that are dead after CCP - both are unnecessary overheads. Alright, I just wanted to think this through - but it seems your approach is good :)

I also considered doing it there in `PhaseCCP::analyze`, but I reached the same conclusion. Thanks for your help!

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

PR Comment: https://git.openjdk.org/jdk/pull/26017#issuecomment-3023978823