[15] RFR: 8242895: failed: sanity at src/hotspot/share/opto/escape.cpp:2361
Jamsheed C M
jamsheed.c.m at oracle.com
Thu Jul 16 01:55:30 UTC 2020
Hi Vladimir,
On 16/07/2020 00:29, Vladimir Kozlov wrote:
> As I said before I agree with your additional checks for StoreN and
> StoreNKlass.
>
> But I have concerns about new is_init_captured_store code. EA is
> mostly looking only on inputs to see Allocation. And in several places
> it expecting only to see Allocation because other cases should be
> filtered out before.
If that is the case, I would like to go with my first webrev for this
fix as it nicely propagate es and there in no unnecessary promotion to
global escape state.
http://cr.openjdk.java.net/~jcm/8242895/webrev_fix_EA/
Best regards,
Jamsheed
>
> Thanks,
> Vladimir
>
> On 7/15/20 10:54 AM, Jamsheed C M wrote:
>> 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
>>>>>>>>>
More information about the hotspot-compiler-dev
mailing list