RFR: 8364970: Redo JDK-8327381 by updating the CmpU type instead of the Bool type [v2]

[Francisco Ferrari Bihurriet]

On Mon, 11 Aug 2025 08:40:53 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Francisco Ferrari Bihurriet has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Apply code review suggestions and add JBS to test
>
> 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.

> src/hotspot/share/opto/subnode.cpp line 855:
> 
>> 853: // (1a) and (1b) is covered by this method since we can directly return the corresponding TypeInt::CC_*
>> 854: // while (2) is covered in BoolNode::Ideal since we create a new non-constant node (see [CMPU_MASK]).
>> 855: const Type* CmpUNode::Value_cmpu_and_mask(PhaseValues* phase, const Node* in1, const Node* in2) {
> 
> I suggest to directly name these:
> `in1` -> `andI`
> `in2`- > `rhs`
> 
> Then it's easier to follow the comments.

Great, I was going to use similar names and later regretted. Suggestion accepted in 27ed1a311ec34e24afae6cc43d2c71e2620eb0ef.

> src/hotspot/share/opto/subnode.cpp line 1899:
> 
>> 1897: // based on local information.   If the input is constant, do it.
>> 1898: const Type* BoolNode::Value(PhaseGVN* phase) const {
>> 1899:   return _test.cc2logical( phase->type( in(1) ) );
> 
> Suggestion:
> 
>   return _test.cc2logical(phase->type(in(1)));

Suggestion accepted in 27ed1a311ec34e24afae6cc43d2c71e2620eb0ef.

> src/hotspot/share/opto/subnode.hpp line 174:
> 
>> 172: // Compare 2 unsigned values (integer or pointer), returning condition codes (-1, 0 or 1).
>> 173: class CmpUNode : public CmpNode {
>> 174:   static const Type* Value_cmpu_and_mask(PhaseValues*, const Node*, const Node*);
> 
> We usually add matching parameter names as found in the source file:
> 
>   static const Type* Value_cmpu_and_mask(PhaseValues* phase, const Node* in1, const Node* in2);
> 
> or with the renaming above:
> 
>   static const Type* Value_cmpu_and_mask(PhaseValues* phase, const Node* andI, const Node* rhs);

Suggestion accepted in 27ed1a311ec34e24afae6cc43d2c71e2620eb0ef.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26666#discussion_r2268462137
PR Review Comment: https://git.openjdk.org/jdk/pull/26666#discussion_r2268467836
PR Review Comment: https://git.openjdk.org/jdk/pull/26666#discussion_r2268468092
PR Review Comment: https://git.openjdk.org/jdk/pull/26666#discussion_r2268468327