<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Mar 13, 2008, at 2:42 PM, Vladimir Kozlov wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">I updated changes and webrev.</div></blockquote><br></div><div>Here's the rest of my review. I hope it helps!</div><div><div><br></div><div>-- John</div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div></div><div>--- escape.cpp</div><div><br class="webkit-block-placeholder"></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Courier" size="4" color="#001afb" style="font: 14.0px Courier; color: #001afb">+ // See comments above.</font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Courier" size="4" color="#001afb" style="font: 14.0px Courier; color: #001afb">+ Node* unique_use = use->raw_out(0);</font></div></div><div><div><br class="webkit-block-placeholder"></div><div>That looks like a factorization opportunity:</div><div> // big comment ...</div><div> Node* find_second_addp(Node* use) { if (...) return use->unique_out(); else return NULL; }</div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Courier" size="4" color="#001afb" style="font: 14.0px Courier; color: #001afb">+ if(!_processed.test(n->_idx) && n->is_AddP())</font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Courier" size="4" color="#001afb" style="font: 14.0px Courier; color: #001afb">+ cg_worklist.append(n->_idx);</font></div></div><br><div><div>I looked but did not find how _processed could be set for the AddP node.</div><div>What sorts of a AddPs are already processed at this early point?</div><div><br class="webkit-block-placeholder"></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Courier" size="4" color="#001afb" style="font: 14.0px Courier; color: #001afb">+void ConnectionGraph::record_for_escape_analysis(Node *n, PhaseTransform *phase) {</font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Courier" size="4" color="#001afb" style="font: 14.0px Courier; color: #001afb">+ switch (n->Opcode()) {</font></div></div><div><br class="webkit-block-placeholder"></div><div>It seems like almost every case in that big switch includes the following:</div><div> ptadr->_node = n;</div><div> <div> ptadr->set_node_type(PointsToNode::(something));</div> <div> set_escape_state(n->_idx, PointsToNode::(something else));</div><div> _processed.set(n->_idx);</div></div><div><br class="webkit-block-placeholder"></div><div>Can those steps be factored out of the switch somehow,</div><div>so it's easy to tell where the irregularities are (if there are any)?</div><div>(It looks like you found on on case Op_LoadKlass.)</div><div>The pattern could be:</div><div><br class="webkit-block-placeholder"></div><div> int ntype = (something default);</div><div> int estate = (something else default);</div><div> bool defer = false;</div><div> _processed.set(n->_idx);</div><div> switch (n->Opcode()) {</div><div> ...</div><div> default: return; // nothing to do</div><div> }</div><div> ptadr->_node = n;</div><div><div><div> ptadr->set_node_type(ntype);</div><div> set_escape_state(n->_idx, estate);</div><div> if (defer) {</div><div> _deferred.push(n);</div><div> _processed.clear(n->_idx);</div><div> }</div><div> return;</div><div><br class="webkit-block-placeholder"></div><div>By the way, the name _deferred confuses me because it makes me think of a kind of CG edge.</div><div>Should be "_delayed_worklist" or "_pass_2_worklist" or some such.</div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Courier" size="4" color="#001afb" style="font: 14.0px Courier; color: #001afb">+ process_call_result(n->as_Proj(), phase);</font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Courier" size="4" color="#001afb" style="font: 14.0px Courier; color: #001afb">+ if (!_processed.test(n->_idx)) {</font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Courier" size="4" color="#001afb" style="font: 14.0px Courier; color: #001afb">+ _deferred.push(n);</font></div><div><font class="Apple-style-span" color="#001AFB" face="Courier" size="4"><span class="Apple-style-span" style="font-size: 14px;"><br class="webkit-block-placeholder"></span></font></div></div><div>Can _processed.test(n) ever be true here?</div><div>(I.e., isn't this an unconditional push to _deferred?)</div><div><br class="webkit-block-placeholder"></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Courier" size="4" color="#001afb" style="font: 14.0px Courier; color: #001afb">+ add_deferred_edge_to_fields(n->_idx, pt, address_offset(adr, phase));</font></div></div><div><br class="webkit-block-placeholder"></div><div>FWIW, address_offset(adr, phase) is loop invariant here.</div><div><br></div></div></div></div></body></html>