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

[Jamsheed C M]

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

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) {

}

[1] 
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/escape.cpp#L2256 

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