RFR: 8364970: Redo JDK-8327381 by updating the CmpU type instead of the Bool type [v2]
Christian Hagedorn
chagedorn at openjdk.org
Wed Aug 13 07:08:24 UTC 2025
On Tue, 12 Aug 2025 03:09:16 GMT, Francisco Ferrari Bihurriet <fferrari at openjdk.org> wrote:
>> src/hotspot/share/opto/phaseX.cpp line 2941:
>>
>>> 2939: // Bool
>>> 2940: //
>>> 2941: void PhaseCCP::push_bool_with_cmpu_and_mask(Unique_Node_List& worklist, const Node* use) const {
>>
>> Needed to double-check but I think it's fine to remove the notification code since we already have `push_cmpu()` in place which looks through the `AddI`:
>> https://github.com/openjdk/jdk/blob/10762d408bba9ce0945100847a8674e7eb7fa75e/src/hotspot/share/opto/phaseX.cpp#L2911-L2926
>>
>> So, whenever `m` or `1` changes, we will re-add the `CmpU` to the CCP worklist with `push_cmpu()`. The `x` does not matter for the application of `Value_cmpu_and_mask()`.
>
> Hmm, I was oversimplifying the problem, my way of thinking it was the following one:
>
>
> m x m 1
> \ / \ /
> AndI AddI grandparents
> \ /
> CmpU parent
> |
> Bool grandchild
>
>
> _"As we were updating a grandchild based on its grandparents, we needed an ad-hoc worklist push for the grandchild. Since we now update the type of `CmpU` based on its parents, the canonical parent-to-child propagations should work, and we don't need any ad-hoc grandparents-to-grandchild worklist push anymore."_
>
> But as you noted, non-immediate `CmpU` inputs such as `m` or `1` can change and should affect the `CmpU` type. Luckily, this already was the case for previous `CmpU` optimizations.
>
> ---
>
> For case **1a**, we don't need `PhaseCCP::push_cmpu` because `m` is also an immediate input of `CmpU`.
>
>
> m x
> \ /
> AndI m
> \ /
> CmpU
> |
> Bool
>
>
> ---
>
> I'm now realizing this was a very lucky situation. The `AndI` input isn't problematic even when `PhaseCCP::push_cmpu` doesn't handle the `use_op == Op_AndI` case, because:
>
> * `x` does not affect the application of `Value_cmpu_and_mask()`
> * In case **1a**, `m` is a direct input of `CmpU`
> * In case **1b**, the `AddI` input is handled in `PhaseCCP::push_cmpu` (`use_op == Op_AddI`)
>
> Please let me know if you think we should add a comment in the code.
That's a good summary! Thanks for double-checking again. It's indeed only for **1b** a probably that's handled by `push_cmpu()`. It probably would not hurt to add a comment that `push_cmpu` handles this case, just to be sure.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26666#discussion_r2272269609
More information about the hotspot-compiler-dev
mailing list