RFR: 8324655: Identify integer minimum and maximum patterns created with if statements [v2]

Emanuel Peter epeter at openjdk.org
Wed Feb 7 09:54:56 UTC 2024


On Wed, 7 Feb 2024 09:41:12 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Jasmine Karthikeyan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Don't transform highly predictable branches
>
> src/hotspot/share/opto/cfgnode.cpp line 1802:
> 
>> 1800: 
>> 1801:   // Ensure comparison is an integral type
>> 1802:   if (opcode == Op_CmpI || opcode == Op_CmpL) {
> 
> Nicer would be "bailout" ifs:
> Suggestion:
> 
>   if (opcode != Op_CmpI && opcode != Op_CmpL) { return nullptr; }
>   bool is_long = opcode == Op_CmpL;
> 
>   // Only accept canonicalized le and lt comparisons
>   int test = bol->_test._test;
>   if (test != BoolTest::le && test != BoolTest::lt) { return nullptr; }
> 
> 
> It is nice to have all the "bailout" checks as early as possible, and then do the "meat" of the optimization afterward.

This also avoids nesting: nesting is difficult to read and understand. A sequence of "bailouts" is easier to read.

> src/hotspot/share/opto/cfgnode.cpp line 1824:
> 
>> 1822:   } else {
>> 1823:     return nullptr;
>> 1824:   }
> 
> Maybe also have variables `phi_true/false`, would make the code easier to read.

Hmm. Maybe you can refactor this, or explain somehow why exactly the conditions lead to `min_path = 1/2`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17574#discussion_r1481189784
PR Review Comment: https://git.openjdk.org/jdk/pull/17574#discussion_r1481208533


More information about the hotspot-compiler-dev mailing list