RFR: 8266854: LibraryCallKit::inline_preconditions_checkIndex modifies control flow even if the intrinsic bailed out [v2]

[Vladimir Ivanov]

On Tue, 11 May 2021 01:06:25 GMT, Sandhya Viswanathan <sviswanathan at openjdk.org> wrote:

>> LibraryCallKit::inline_preconditions_checkIndex can result in the following assert sometimes:
>>    "# assert(ctrl == kit.control()) failed: Control flow was added although the intrinsic bailed out"
>> 
>> Consider the following code snippet:
>>  ...
>>  set_control(_gvn.transform(new IfTrueNode(rc)));
>>  {
>>    PreserveJVMState pjvms(this);
>>    set_control(_gvn.transform(new IfFalseNode(rc)));
>>    uncommon_trap(Deoptimization::Reason_range_check,
>>                  Deoptimization::Action_make_not_entrant);
>>  }
>>  ..
>>  Here the control is being modified by set_control even though a bailout is possible afterwards.
>>  Moving the set_control later in the intrinsic fixes this.
>> 
>>  This is a small fix. Please review.
>> 
>> Best Regards,
>> Sandhya
>
> Sandhya Viswanathan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Implement review comments from Vladimir Kozlov

src/hotspot/share/opto/library_call.cpp line 1057:

> 1055:   }
> 1056: 
> 1057:   if (stopped()) {

Moving the check around doesn't make much sense to me.

`stopped() == false` signals that the current control is effectively dead. It could happen when the range check (`RangeCheckNode` )  always fails and execution unconditionally hits the uncommon trap. (I haven't double-checked myself whether it happens in practice or not.) 

By bailing out from the intrinsic (`return false;`), the next thing C2 will attempt is to inline `Preconditions::checkIndex(). There were no attempts to clean up the graph built up to this point (with the uncommon trap).

Instead, the fix can just be `if (stopped()) { return true; }`. The graph constructed so far is valid.

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

PR: https://git.openjdk.java.net/jdk/pull/3958