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

[Sandhya Viswanathan]

On Tue, 11 May 2021 12:59:00 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> 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.

'if (stopped()) { return true; }'  (i.e. only changing the return false to return true at original line 1058) also fixes the issue.
It is something to do with range check fails. 

I discovered this issue while working on vector api. I had a call to Objects.checkIndex(origin, length()) and came across the 'assert(ctrl == kit.control()) failed'.  As part of code review we found that the call should have been Objects.checkIndex(origin, length()+1) so it looks like something to do with range check fails. I am unable to create a stand alone test case.  

Let me know if we should go ahead with the 'if (stopped()) { return true; }' fix.

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

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