RFR(S): 8238723: yank_alloc_node must remove membar
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Fri Feb 21 17:44:51 UTC 2020
> http://cr.openjdk.java.net/~neliasso/8238723/webrev.04/
>
> The tests now run with -DoEscapeAnalysis too. One removal was logged
> from the testStoreCapture testcase.
>
> 1083 87 b 4 compiler.allocation.TestAllocation::testStoreCapture (11 bytes)
> NotUsed 37 AllocateArray === 5 6 7 8 1 ( 33 23 28 22 1 1 ) [[ 38 39 40 47 48 ]] rawptr:NotNull ( int:>=0, java/lang/Object:NotNull *, bool, int ) TestAllocation::testStoreCapture @ bci:1 !jvms: TestAllocation::testStoreCapture @ bci:1
> ++++ Eliminated: 37 AllocateArray
Good!
Best regards,
Vladimir Ivanov
> On 2020-02-21 16:41, Vladimir Ivanov wrote:
>>
>>> http://cr.openjdk.java.net/~neliasso/8238723/webrev.03
>>
>> Looks good.
>>
>> Also, please, incorporate the following patch [1] which extends
>> -XX:+PrintEliminateAllocations output.
>>
>> FTR the following simple test case demonstrates store capturing in
>> action for AllocateArray (w/ -XX:-DoEscapeAnalysis):
>>
>> static void testAllocArray0() {
>> byte[] bs = new byte[1];
>> bs[0] = 1;
>> }
>>
>> The reason it doesn't break your patch is that AddPs are eliminated
>> during Initialize node removal:
>> PhaseIterGVN::remove_globally_dead_node() (called through [2])
>> aggressively prunes dead inputs and kills Stores and AddPs right away.
>>
>> PS: it would be good to add -XX:-DoEscapeAnalysis configuration in the
>> test, but currently it doesn't have any cases which demonstrate unused
>> allocation elimination w/o EA turned on [3].
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> [1]
>> diff --git a/src/hotspot/share/opto/macro.cpp
>> b/src/hotspot/share/opto/macro.cpp
>> --- a/src/hotspot/share/opto/macro.cpp
>> +++ b/src/hotspot/share/opto/macro.cpp
>> @@ -1334,6 +1334,17 @@
>> init->remove(&_igvn);
>> }
>> if (expand_fast_path && (initial_slow_test == NULL)) {
>> +#ifndef PRODUCT
>> + if (PrintEliminateAllocations) {
>> + tty->print("NotUsed ");
>> + Node* res = alloc->proj_out_or_null(TypeFunc::Parms);
>> + if (res != NULL) {
>> + res->dump();
>> + } else {
>> + alloc->dump();
>> + }
>> + }
>> +#endif
>> // Remove allocation node and return.
>> // Size is a non-negative constant -> no initial check needed
>> -> directly to fast path.
>> // Also, no usages -> empty fast path -> no fall out to slow
>> path -> nothing left.
>> @@ -1614,6 +1625,15 @@
>> _ioproj_catchall->set_req(0, top());
>> }
>> _igvn.remove_dead_node(alloc);
>> +
>> +#ifndef PRODUCT
>> + if (PrintEliminateAllocations) {
>> + if (alloc->is_AllocateArray())
>> + tty->print_cr("++++ Eliminated: %d AllocateArray", alloc->_idx);
>> + else
>> + tty->print_cr("++++ Eliminated: %d Allocate", alloc->_idx);
>> + }
>> +#endif
>> }
>>
>> void PhaseMacroExpand::expand_initialize_membar(AllocateNode* alloc,
>> InitializeNode* init,
>>
>>
>> [2] MemBarNode::remove =>
>> PhaseIterGVN::replace_node =>
>> PhaseIterGVN::subsume_node =>
>> PhaseIterGVN::remove_dead_node =>
>> PhaseIterGVN::remove_globally_dead_node
>>
>> [3]
>> $ java -cp JTwork/classes/compiler/allocation/TestAllocation.d/
>> -Xbatch
>> -XX:CompileCommand=compileonly,compiler.allocation.TestAllocation::*
>> -XX:-TieredCompilation -XX:CompileCommand=quiet -XX:+PrintCompilation
>> -XX:+PrintInlining -XX:-DoEscapeAnalysis -XX:+PrintEscapeAnalysis
>> -XX:+PrintEliminateAllocations compiler/allocation/TestAllocation
>> CompileCommand: compileonly compiler/allocation/TestAllocation.*
>> 1488 1 b
>> compiler.allocation.TestAllocation::testUnknownPositiveArrayLength (24
>> bytes)
>> @ 9
>> compiler.allocation.TestAllocation$Payload::<init> (17 bytes) many
>> throws
>> @ 1 java.lang.Object::<init> (1 bytes)
>> inline (hot)
>> 1502 2 !b
>> compiler.allocation.TestAllocation::testUnknownNegativeArrayLength (45
>> bytes)
>> @ 11
>> compiler.allocation.TestAllocation$Payload::<init> (17 bytes) many
>> throws
>> @ 1 java.lang.Object::<init> (1 bytes)
>> inline (hot)
>> 1516 3 b
>> compiler.allocation.TestAllocation::testConstantPositiveArrayLength
>> (24 bytes)
>> @ 9
>> compiler.allocation.TestAllocation$Payload::<init> (17 bytes) many
>> throws
>> @ 1 java.lang.Object::<init> (1 bytes)
>> inline (hot)
>> 1527 4 !b
>> compiler.allocation.TestAllocation::testConstantNegativeArrayLength
>> (45 bytes)
>> @ 11
>> compiler.allocation.TestAllocation$Payload::<init> (17 bytes) many
>> throws
>> @ 1 java.lang.Object::<init> (1 bytes)
>> inline (hot)
>>
>>> Best Regards,
>>>
>>> Nils Eliasson
>>>
>>>
>>> On 2020-02-20 16:59, Vladimir Ivanov wrote:
>>>>
>>>>> And to address your concern on AddPs - I still haven't seen an
>>>>> example on when they occur. How would the graph look? What is using
>>>>> the addP? So far I have been content with asserting on addPs, and
>>>>> still hasn't found any occurrence in our testing. But I want to be
>>>>> safe so I will change to skip the allocation removal if there is an
>>>>> addP (but keep asserting in debug builds)
>>>>
>>>> InitializeNode attemps to convert stores into freshly allocated
>>>> object into "initializing stores" to avoid redundant zeroing of the
>>>> contents (see InitializeNode::capture_store() [1]). The comment
>>>> provides an example of IR shape:
>>>>
>>>> // alloc = (Allocate ...)
>>>> // rawoop = alloc.RawAddress
>>>> // rawstore1 = (StoreC alloc.Control alloc.Memory (+ rawoop 12) 1)
>>>> // rawstore2 = (StoreC alloc.Control alloc.Memory (+ rawoop 14) 2)
>>>> // init = (Initialize alloc.Control alloc.Memory rawoop
>>>> // rawstore1 rawstore2)
>>>>
>>>> Those stores should go away along with InitializeNode, so it should
>>>> be safe to unlink them right away. So, I don't see a compelling
>>>> reason to skip allocation removal in such case.
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/memnode.cpp#l3803
>>>>
>>>>
>>>>>
>>>>> // Nils
>>>>>
>>>>>
>>>>>
>>>>> [1]
>>>>> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/parse1.cpp#l1010
>>>>>
>>>>>
>>>>>>
>>>>>> Sometimes MemBarRelease is added [1] which can degenerate into
>>>>>> MemBarCPUOrder [2].
>>>>>>
>>>>>> I'd prefer to see all the barriers which are linked to the
>>>>>> allocation be removed.
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>>
>>>>>> [1]
>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/parse1.cpp#l1005
>>>>>>
>>>>>>
>>>>>> [2]
>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/escape.cpp#l1912
>>>>>>
>>>>>>
>>>>>>> On 2020-02-18 14:01, Tobias Hartmann wrote:
>>>>>>>> On 18.02.20 13:32, Vladimir Ivanov wrote:
>>>>>>>>> In case of MemBar I assume you are handling the precedence edge
>>>>>>>>> (MemBarNode::Precedent). Why do you
>>>>>>>>> replace it with TOP? I assume you want to eliminate the
>>>>>>>>> barrier, but I don't see how it helps.
>>>>>>>> Actually, you might want to use MemBarNode::remove.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Tobias
>>>>>
>
More information about the hotspot-compiler-dev
mailing list