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

[John Rose]

On Dec 20, 2012, at 12:56 PM, Roland Westrelin wrote:

> http://cr.openjdk.java.net/~roland/8005071/webrev.01/


This is shaping up very well.  Here are some more comments:

Suggest a more descriptive name:  s/update_with/replace_with/

For updating a boolean field (_inlining_incrementally, _inlining_progress), it is at least 5x more common to use the form "set_foo(bool z)" than a combination of "set_foo()" and "clear_foo()".  I suggest using a boolean parameter instead of a set/clear pair.  (A quick and dirty grep catches 125 "set_foo(bool z) {...}", 26 "set_foo() {...true...}", and 2 "clear_foo() {...false...}".)

Consider adding an accessor function CallNode::set_cg, and maybe also a getter cg().   There's a lot of '->_cg'; it might look better with member functions.  Perhaps s/cg/generator/.

Basically, field accessors should have really uninteresting names, "foo" and "set_foo" for "_foo".  See:
  https://wikis.oracle.com/display/HotSpotInternals/StyleGuide#StyleGuide-Naming

There are a few places where you omit the space after "if" and "for".  The majority usage is to include spaces: "if (" and "for (".  New code should have spaces.  (I added a line about this to the StyleGuide page.)

Similar comment about 'Node *n' vs. 'Node* n'.  The latter is preferable, and we should be going for reasonable consistency in any case.

Some of the lines need a good sprinkle of whitespace.  For example:
-   for(uint i=1; i<phi->req(); ++i) {
+   for (uint i = 1; i < phi->req(); ++i) {

In compile.cpp, the expression 'n->isa_Call() != NULL' can be simplified to 'n->is_Call()'.

With remove_useless_late_inlines, I suggest passing 'GrowableArray<CallGenerator*>* inlines' as an operand, instead of a magic boolean.  And it should probably be the first operand, so that the output value is to the right of the input.

Is there a way to name 'Compile::string_inline' as a verb phrase?  Like 'inline_string_calls'?  Same question about 'incremental_inline_one' and 'incremental_inline'.  Maybe 'inline_incrementally' and 'inline_incrementally_once'.

In CallNode::Ideal, you can replace Compile::current() by phase->C.

Suggest guarding or asserting with MethodHandles::has_member_arg in 'else' of CallNode::Ideal, since it is a trailing member argument that you are checking for ConP.

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.

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!

Perhaps an alternative transformation (that would be more safe) would be to replace this:
  if (top) {foo} else {bar}
with this:
  if (false) {foo} else {bar}

After all, if the code is not dead, but the predicate tops out, then we are allowed to choose an arbitrary branch of the if.
(In order to prevent indeterminacy, both foo and bar should have exactly the same effects, but that is impossible to prove in general, and we may hope it is true.)

— John