[15] RFR: 8242895: failed: sanity at src/hotspot/share/opto/escape.cpp:2361

[Jamsheed C M]

Hi Vladimir,

with unrolling i understand that many cases will just have phis 
everywhere to outside the loop as the uses are outside the loop.

and this is not restricted to escaping objects alone as i depicted. it 
can be escaping as well as non-escaping.

so marking store to them as global escape doesn't seems to be nice idea. 
i will rework on this fix and get back again.

Thank you

Best regards

Jamsheed

On 15/07/2020 08:38, Jamsheed C M wrote:
> (unfinished mail got sent, so completing it)
> On 15/07/2020 08:21, Jamsheed C M wrote:
>> Hi Vladimir,
>>
>> On 15/07/2020 06:50, Vladimir Kozlov wrote:
>>> I looked more on this. EA already does not secularize allocations 
>>> when Phi nodes merged them - it should handle this case. I did small 
>>> experiment and relaxed assert for this new (10. needs comment 
>>> update) case for AddP's base and test passed:
>>>
>>> src/hotspot/share/opto/escape.cpp Tue Jul 14 18:11:27 2020 -0700
>>> @@ -2357,6 +2357,7 @@
>>>        int opcode = uncast_base->Opcode();
>>>        assert(opcode == Op_ConP || opcode == Op_ThreadLocal ||
>>>               opcode == Op_CastX2P || 
>>> uncast_base->is_DecodeNarrowPtr() ||
>>> +             (uncast_base->is_Phi() && 
>>> (uncast_base->bottom_type()->isa_rawptr() != NULL)) ||
>>>               (uncast_base->is_Mem() && 
>>> (uncast_base->bottom_type()->isa_rawptr() != NULL)) ||
>>>               (uncast_base->is_Proj() && 
>>> uncast_base->in(0)->is_Allocate()), "sanity");
>>>      }
>>>
>>> Did you hit a case when this may not work?
>>
>> Yes, right it already doesn't mark it as scalarizable if base count 
>> is more than one(I think it missed a is_oop check there)[1].
>>
>> EA CG adds edges only for oop field making stores to them undetected. 
>> This makes these stored objects to NoEscape and if compiled method 
>> continues execution with this NoEscape object can have undesired 
>> results(i.e synchronization removed).
>>
>> Probable case would be(didn't verify)
>>
>> try {
>>
>> LOOP BEGIN
>>
>>   try {throw new Obj()} catch {}
>>
>> LOOP END
>>
>> } catch (Obj e) {
>>
>> }
>
> Best Regards,
>
> Jamsheed
>
> [1]https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/escape.cpp#L1770 
>
>
>
>>>
>>>
>>> And with LoopOpts off -XX:LoopUnrollLimit=0 it removed allocation 
>>> (-XX:+PrintEscapeAnalysis -XX:+PrintEliminateAllocations):
>>>
>>> ======== Connection graph for  Test::test
>>> JavaObject NoEscape(NoEscape) [ 158F [ 107 ]]   95    Allocate ===  
>>> 242  76  230  8  1 ( 93  92  21  1  78  1  78 ) [[ 96 97 98 105  
>>> 106  107 ]]  rawptr:NotNull ( int:>=0, java/lang/Object:NotNull *, 
>>> bool, top ) Test::test1 @ bci:0 Test::test @ bci:8 !jvms: 
>>> Test::test1 @ bci:0 Test::test @ bci:8
>>> LocalVar [ 95P [ 158b ]]   107    Proj    ===  95  [[ 108  158 ]] #5 
>>> !jvms: Test::test1 @ bci:0 Test::test @ bci:8
>>>
>>> Scalar  95    Allocate    ===  242  76  230  8  1 ( 93  92  21 1 78  
>>> 1  78 ) [[ 96  97  98  105  106  107 ]]  rawptr:NotNull ( int:>=0, 
>>> java/lang/Object:NotNull *, bool, top ) Test::test1 @ bci:0 
>>> Test::test @ bci:8 !jvms: Test::test1 @ bci:0 Test::test @ bci:8
>>> ++++ Eliminated: 95 Allocate
>>>
>>>
>>> t\Thanks,
>>> Vladimir K
>>>
>>> On 7/14/20 1:28 AM, Jamsheed C M wrote:
>>>> Hi all,
>>>>
>>>> I had incorrectly added extra check in assert after offset 
>>>> computation in address_offset . For addps with non constant offsets 
>>>> (like [1])
>>>>
>>>> Not changing the old assert even though I am not expecting first 
>>>> addp/second addp(for array addressing) case for init captured store.
>>>>
>>>> http://cr.openjdk.java.net/~jcm/8242895/webrev_fix_EA_asserts_corrected/ 
>>>>
>>>>
>>>> Best regards,
>>>>
>>>> Jamsheed
>>>>
>>>> [1]
>>>>
>>>> assert(offs != Type::OffsetBot ||
>>>> - adr->in(AddPNode::Address)->in(0)->is_AllocateArray(),
>>>> + adr->in(AddPNode::Address)->in(0)->is_AllocateArray() || 
>>>> is_captured_store(adr),
>>>>              "offset must be a constant or it is initialization of 
>>>> array");
>>>>
>>>> On 13/07/2020 11:14, Jamsheed C M wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I reworked the fix. I compute offset for all init captures stores, 
>>>>> but treats this special init captured stores similar to unsafe(as 
>>>>> these objects are usually GlobalEscape and doesn't have any perf 
>>>>> implications).
>>>>>
>>>>> revised webrev: 
>>>>> http://cr.openjdk.java.net/~jcm/8242895/webrev_fix_EA.01/
>>>>>
>>>>> testing: mach1-5( logs in jbs)
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Jamsheed
>>>>>
>>>>> On 09/07/2020 19:36, Jamsheed C M wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> request to hold the review. need to change the code for dealing 
>>>>>> with unsafe access. as current capture code go for more execution 
>>>>>> time analyzing things.
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Jamsheed
>>>>>>
>>>>>> On 09/07/2020 13:01, Jamsheed C M wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8242895
>>>>>>>
>>>>>>> Request for review changes made to offset computation and field 
>>>>>>> write detection for init captured stores due to phis addition 
>>>>>>> between alloc and init. This happen if init node in different 
>>>>>>> outer loop wrt to alloc node and there is a loop opt.  This was 
>>>>>>> required as a result of enhancement [1].
>>>>>>>
>>>>>>> Normally init are not associated with multiple alloc node during 
>>>>>>> EA phase, but changes done for [1] caused the code shapes of the 
>>>>>>> form [2]  to generate inits associated with multiple alloc node.
>>>>>>>
>>>>>>> This had implication in offset computation and field write 
>>>>>>> detection related to initializing stores.
>>>>>>>
>>>>>>> Attempt to fix in EA:
>>>>>>>
>>>>>>>      webrev: http://cr.openjdk.java.net/~jcm/8242895/webrev_fix_EA/
>>>>>>>
>>>>>>> Alternate fix:
>>>>>>>
>>>>>>>      Minimize the scenario in compiler generated code by 
>>>>>>> throwing only j.l.Error from slowpath(all exception async/sync 
>>>>>>> are handled in runtime exit).
>>>>>>>
>>>>>>>      Stub epilog doesn't poll or throw any exceptions. Disable 
>>>>>>> full loop opt before EA for detectable patterns and bailout EA 
>>>>>>> for late detected patterns.
>>>>>>>
>>>>>>>      webrev: http://cr.openjdk.java.net/~jcm/8242895/webrev_deopt/
>>>>>>>
>>>>>>> Please advice.
>>>>>>>
>>>>>>> Testing : mach tier1-5 (logs in jbs)
>>>>>>>
>>>>>>> Best regards,
>>>>>>>
>>>>>>> Jamsheed
>>>>>>>
>>>>>>>
>>>>>>> [1] JDK-8231291 
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8231291>C2: loop opts 
>>>>>>> before EA should maximally unroll loops
>>>>>>>
>>>>>>> [2] that have its init node in different outer loop wrt to alloc 
>>>>>>> node.
>>>>>>>
>>>>>>>
>>>>>>> loop begin
>>>>>>>
>>>>>>>    try{
>>>>>>>
>>>>>>>    return new obj()/  throw new obj()/ uncommon trap after 
>>>>>>> allocation, in a loop
>>>>>>>
>>>>>>>    } catch(ex) {
>>>>>>>
>>>>>>>    }
>>>>>>>
>>>>>>> loop end
>>>>>>>
>>>>>>>   42     public static IntA test(int n) {
>>>>>>>    43         for (int i=0; i<2; i++) {
>>>>>>>    44             try {
>>>>>>>    45                   return new IntA(n + i);
>>>>>>>    46             } catch (Exception e) {
>>>>>>>    47             }
>>>>>>>    48         }
>>>>>>>    49
>>>>>>>