[15] RFR: 8242895: failed: sanity at src/hotspot/share/opto/escape.cpp:2361
Jamsheed C M
jamsheed.c.m at oracle.com
Wed Jul 15 03:08:10 UTC 2020
(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
>>>>>>
More information about the hotspot-compiler-dev
mailing list