RFR(S): 8238723: yank_alloc_node must remove membar
Nils Eliasson
nils.eliasson at oracle.com
Fri Feb 21 17:22:23 UTC 2020
I added all your suggestions:
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
Best regards,
Nils Eliasson
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