RFR(S): 8238723: yank_alloc_node must remove membar

[Vladimir Ivanov]

> 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
>>>