[15] RFR: 8242895: failed: sanity at src/hotspot/share/opto/escape.cpp:2361
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Jul 15 18:59:40 UTC 2020
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.
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