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

[Roland Westrelin]

Hi John,

Thanks for the review.

I'm updating the code following your suggestions and I'll send an updated webrev later today.

> In RegionNode::try_clean_mem_phi, it appears you can get a SEGV if your Region has no Phi nodes at all.
> 
> Actually, you can remove the loop, and use the utility function RegionNode::has_unique_phi.
> 
> The boolean return value of try_clean_mem_phi is confusing.  It should be documented, or else inverted to be a more intuitive "success" indicator:
> 
> +    if (has_phis && try_clean_mem_phi(phase)) {
> +      has_phis = false;  // cleaned out all phis
> +    }
> 
> There are two declarations of the index variable 'uint i', and one shadows the other.  This may cause warnings on some compilers, and is (anyway) confusing.  Use 'j' or 'i2' for the nested guy.
> 
> In fact, the nested loops are confusing.  I think you can do the job more clearly (and with more comments, please) in two consecutive loops, one to find the first MergeMemNode and, then another loop to check that the other edges can be discarded.

As Vladimir suggested there's no need for 2 loops because we only have 2 inputs to the Phi and if one looks good for the transformation, we just want to look at the other one.

> The way I read the code, the memory phi must have inputs which are limited to NULL, a unique MergeMem m (with an out-count of 1, meaning the Phi is the only consumer), or m's base-memory (which must not itself be a MergeMem, for some reason I don't get).  Given such a Phi, it follows that the Region node represents the merge of code like this:
> 
>  if (predicate) {
>    memory.x = xval;
>    //and maybe more memory2.y = yval...
>  }
> 
> In this case, the MergeMem collects one or more side effects.  And your transformation replaces the Phi by an unconditional memory update:
> 
>  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.

Roland.