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

[Vladimir Kozlov]

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?

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