RFR: 8335221: Some C2 intrinsics incorrectly assume that type argument is compile-time constant

[Vladimir Kozlov]

On Thu, 27 Jun 2024 09:21:16 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Problems were found in some C2 intrinsics while testing Leyden Early Release.
>> 
>> [`inline_array_partition()`](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/library_call.cpp#L5493) and [`inline_array_sort()`](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/library_call.cpp#L5559) intrinsics incorrectly assume that `elementType` argument is always compile-time constant. Which is not true for Leyden (and in general) when constant folding optimization is switched off (or did not executed for particular type).
>> 
>> Note, other intrinsics are very careful about that. For example, see [`inline_Class_cast()`](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/library_call.cpp#L3963) intrinsic.
>> 
>> Add similar checks to `inline_array_partition()` and `inline_array_sort()`.
>> 
>> An other problem is that `null_check()` (which adds new control nodes to graph) was executed in these intrinsics before bailouts from intrinsic generation. We hit next assert if bailout happens:
>> 
>>   assert(ctrl == kit.control(), "Control flow was added although the intrinsic bailed out");
>> 
>> 
>> I moved `null_check()` after bailouts.
>> 
>> An other issue with `null_check()` is it checks for `null` first argument which is `Class`. It can't be `null` because corresponding Java methods are `private` and each call site passed `<primitive_type>.class` as first argument.
>> For example: [DualPivotQuicksort.java#L260](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/DualPivotQuicksort.java#L260)
>> On other hand there is no null check for passed array (second argument). I changed `null_check()` for array.
>> 
>> These 2 intrinsics were added by [JDK-8309130](https://bugs.openjdk.org/browse/JDK-8309130) in JDK 22.
>> 
>> Testing tier1-6, hs-stress, hs-xcomp
>
> src/hotspot/share/opto/library_call.cpp line 5523:
> 
>> 5521:   }
>> 5522:   address stubAddr = nullptr;
>> 5523:   stubAddr = StubRoutines::select_array_partition_function();
> 
> Can be merged:
> Suggestion:
> 
>   address stubAddr = StubRoutines::select_array_partition_function();

done

> src/hotspot/share/opto/library_call.cpp line 5530:
> 
>> 5528:   // get the address of the array
>> 5529:   const TypeAryPtr* obj_t = _gvn.type(obj)->isa_aryptr();
>> 5530:   if (obj_t == nullptr || obj_t->elem() == Type::BOTTOM ) {
> 
> Suggestion:
> 
>   if (obj_t == nullptr || obj_t->elem() == Type::BOTTOM) {

done

> src/hotspot/share/opto/library_call.cpp line 5532:
> 
>> 5530:   if (obj_t == nullptr || obj_t->elem() == Type::BOTTOM ) {
>> 5531:     return false; // failed input validation
>> 5532:   }
> 
> The bailout code looks almost identical to the one in `inline_array_sort()`. Can it somehow be shared?

Good idea. I will try.

> src/hotspot/share/opto/library_call.cpp line 5536:
> 
>> 5534:   null_check(obj);
>> 5535:   // If obj is dead, only null-path is taken.
>> 5536:   if (stopped())  return true;
> 
> Can you add braces here?
> Suggestion:
> 
>   if (stopped()) {
>     return true;
>   }

done

> src/hotspot/share/opto/library_call.cpp line 5614:
> 
>> 5612:   null_check(obj);
>> 5613:   // If obj is dead, only null-path is taken.
>> 5614:   if (stopped())  return true;
> 
> Suggestion:
> 
>   if (stopped()) { 
>     return true;
>   }

done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19918#discussion_r1657414336
PR Review Comment: https://git.openjdk.org/jdk/pull/19918#discussion_r1657415987
PR Review Comment: https://git.openjdk.org/jdk/pull/19918#discussion_r1657420728
PR Review Comment: https://git.openjdk.org/jdk/pull/19918#discussion_r1657414575
PR Review Comment: https://git.openjdk.org/jdk/pull/19918#discussion_r1657414902