RFR(L): 8005071: Incremental inlining for JSR 292

John Rose john.r.rose at oracle.com
Sat Dec 22 15:49:10 PST 2012


On Dec 21, 2012, at 5:41 AM, Roland Westrelin wrote:

>> if (predicate) { }
>> if (true) {
>>   memory.x = xval;
>>   //and maybe more memory2.y = yval...
>> }
>> 
>> I think this code needs more commenting to explain why it is a safe transformation.  It actually seems unsafe to me!
> 
> The outcnt() == 1 test is on the region's i input for the Phi's i input which guarantees, I think, that the region is the only use for the If projection and so there's nothing in this branch of the If and so nothing that updates memory and is an input to the MergeMem. So your if example above is not transformed.

I see.  (I misread whose outcnt was being tested.)  I suggest comments like:

+      if (ii && ii->is_MergeMem() && in(i)->outcnt() == 1) {
++       // Nothing is control-dependent on path #i except the region itself.
+        m = ii->as_MergeMem();
+        uint j = 3 - i;
+        Node* other = phi->in(j);
+        if (other && other == m->base_memory()) {
++         // m is a successor memory to other, and is not pinned inside the diamond, so push it out.
++         // This will allow the diamond to collapse completely.
+          phase->is_IterGVN()->replace_node(phi, m);
+          return true;
+        }

Also suggest:

++    assert(phi->req() == 3, "same as region");
++    for (uint i = 1; i < 3; ++i) {

That makes the 'j = 3 - i' easier to follow.

Also, since 'ii' is a memory state, call it something more descriptive, like 'mem'.

On Dec 21, 2012, at 5:46 AM, Roland Westrelin wrote:

>> 
>> Also use phi->as_Phi()->is_diamond_phi() instead of region's inputs checks.
> 
> I don't think that would work in every cases. Because one of the input is top, the CmpP, if it's ahead of the Region in the igvn's worklist, will go away and be replaced by top and then when the Region is processed, phi->as_Phi()->is_diamond_phi() won't return true.


I sympathize with Vladimir here, since this thing is hard to follow:  in(1) && in(2) && in(1)->in(0) == in(2)->in(0)

I suggest adding an optional boolean option to is_diamond_phi:

// If check_control_only is true, do not inspect the If node at the top, and return -1 (not an edge number) on success.
int PhiNode::is_diamond_phi(bool check_control_only = false) const {
  // Check for a 2-path merge
  Node *region = in(0);
  if( !region ) return 0;
  if( region->req() != 3 ) return 0;
  if(         req() != 3 ) return 0;
  // Check that both paths come from the same If
  Node *ifp1 = region->in(1);
  Node *ifp2 = region->in(2);
  if( !ifp1 || !ifp2 ) return 0;
  if (check_control_only)  return (ifp1 == ifp2) ? -1 : 0;

That way the pattern match can build on the useful concept of a diamond.

— John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20121222/fab90c65/attachment.html 


More information about the hotspot-compiler-dev mailing list