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

[Vladimir Kozlov]

On Tue, 11 May 2021 18:16:52 GMT, Sandhya Viswanathan <sviswanathan at openjdk.org> wrote:

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

Yes, returning `true` could be correct because of constant folding we end up in uncommon trap which is valid for provided values (constants). It will be the same if code is compiled normally.
But we also need to move the other `stopped()` check from line 1036 to 1028 after BuildCutout which may also can go to uncommon trap (when length is know negative during compilation). And it also should return `true`.

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

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