<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">
I’m not sure why the web updates are not reflecting in the email. Updated the thread about an hour back through phab:
<div class=""><a href="https://reviews.llvm.org/D23920" class="">https://reviews.llvm.org/D23920</a></div>
<div class=""><br class="">
</div>
<div class=""><br class="">
</div>
<div class="">
<p style="margin: 0px 0px 12px; padding: 0px; border: 0px; color: rgb(70, 76, 92); font-family: 'Segoe UI', 'Segoe UI Web Regular', 'Segoe UI Symbol', 'Helvetica Neue', Helvetica, Arial, sans-serif; font-size: 13px;" class="">
I think base_value and current_value are the same in the example below:</p>
<div class="remarkup-code-block" data-code-lang="text" data-sigil="remarkup-code-block" style="margin: 12px 0px; padding: 0px; border: 0px; white-space: pre; color: rgb(70, 76, 92); font-family: 'Segoe UI', 'Segoe UI Web Regular', 'Segoe UI Symbol', 'Helvetica Neue', Helvetica, Arial, sans-serif; font-size: 13px;">
<pre class="remarkup-code" style="margin-top: 0px; margin-bottom: 0px; padding: 8px; border: 1px solid rgb(233, 219, 205); background-color: rgb(253, 243, 218); color: rgb(0, 0, 0); overflow: auto; font-family: Menlo, Consolas, Monaco, monospace; font-size: 10px; line-height: normal;">base_value = phi (%a, BB1), (%b, BB2)
current_value = phi (%a, BB2), (%b, BB1)</pre>
</div>
<p style="margin: 0px 0px 12px; padding: 0px; border: 0px; color: rgb(70, 76, 92); font-family: 'Segoe UI', 'Segoe UI Web Regular', 'Segoe UI Symbol', 'Helvetica Neue', Helvetica, Arial, sans-serif; font-size: 13px;" class="">
Although the value is coming from different basic blocks, since the incoming value is an SSA value, <tt class="remarkup-monospaced" style="color: rgb(51, 51, 51); background-color: rgb(235, 235, 235); padding: 0px 4px; white-space: pre-wrap; background-position: initial initial; background-repeat: initial initial;">(%a,
BB1)</tt> is same as <tt class="remarkup-monospaced" style="color: rgb(51, 51, 51); background-color: rgb(235, 235, 235); padding: 0px 4px; white-space: pre-wrap; background-position: initial initial; background-repeat: initial initial;">(%a, BB2)</tt>. There
is only one definition for <tt class="remarkup-monospaced" style="color: rgb(51, 51, 51); background-color: rgb(235, 235, 235); padding: 0px 4px; white-space: pre-wrap; background-position: initial initial; background-repeat: initial initial;">%a</tt>, and
all uses are dominated by the def. So, <tt class="remarkup-monospaced" style="color: rgb(51, 51, 51); background-color: rgb(235, 235, 235); padding: 0px 4px; white-space: pre-wrap; background-position: initial initial; background-repeat: initial initial;">base_value</tt> == <tt class="remarkup-monospaced" style="color: rgb(51, 51, 51); background-color: rgb(235, 235, 235); padding: 0px 4px; white-space: pre-wrap; background-position: initial initial; background-repeat: initial initial;">current_value</tt> in
the example. <br class="">
Another concern that came to mind as I saw the example was, if we needed a recursive check if <tt class="remarkup-monospaced" style="color: rgb(51, 51, 51); background-color: rgb(235, 235, 235); padding: 0px 4px; white-space: pre-wrap; background-position: initial initial; background-repeat: initial initial;">%a</tt> was
also a phi node. Again, we do not need this check, for the same reason that <tt class="remarkup-monospaced" style="color: rgb(51, 51, 51); background-color: rgb(235, 235, 235); padding: 0px 4px; white-space: pre-wrap; background-position: initial initial; background-repeat: initial initial;">%a</tt> is
an SSA value.</p>
<div style="margin: 0px; padding: 0px; border: 0px; color: rgb(70, 76, 92); font-family: 'Segoe UI', 'Segoe UI Web Regular', 'Segoe UI Symbol', 'Helvetica Neue', Helvetica, Arial, sans-serif; font-size: 13px;" class="">
Regarding the second example, I initially had this in mind while writing the patch, but the check complexity is O(n log n) for n = <tt class="remarkup-monospaced" style="color: rgb(51, 51, 51); background-color: rgb(235, 235, 235); padding: 0px 4px; white-space: pre-wrap; background-position: initial initial; background-repeat: initial initial;">PhiNum</tt>.
Didnt think it warranted the expense :) I'll add this case.</div>
<div style="margin: 0px; padding: 0px; border: 0px; color: rgb(70, 76, 92); font-family: 'Segoe UI', 'Segoe UI Web Regular', 'Segoe UI Symbol', 'Helvetica Neue', Helvetica, Arial, sans-serif; font-size: 13px;" class="">
<br class="">
</div>
<div>
<blockquote type="cite" class="">
<div class="">On Aug 26, 2016, at 10:48 AM, Igor Laevsky <<a href="mailto:igor@azulsystems.com" class="">igor@azulsystems.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">igor-laevsky requested changes to this revision.<br class="">
igor-laevsky added a comment.<br class="">
This revision now requires changes to proceed.<br class="">
<br class="">
Could you please add a couple of tests? Also you should update comment in the header of the "findRematerializableChainToBasePointer".<br class="">
<br class="">
<br class="">
================<br class="">
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1816-1817<br class="">
@@ -1815,1 +1815,4 @@<br class="">
<br class="">
+ // PHI nodes that have the same incoming values, and belonging to the same<br class="">
+ // basic blocks are essentially the same SSA value.<br class="">
+ if (PHINode *PhiCurr = dyn_cast<PHINode>(CurrentValue))<br class="">
----------------<br class="">
I would emphasise in the comment that it's a termination condition, not a continuation of recursion. Also would be nice to move it closer to the "CurrentValue == BaseValue" case.<br class="">
<br class="">
================<br class="">
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1820<br class="">
@@ +1819,3 @@<br class="">
+ if (PHINode *PhiBase = dyn_cast<PHINode>(BaseValue)) {<br class="">
+ int phiNum = PhiCurr->getNumIncomingValues();<br class="">
+ if (phiNum != PhiBase->getNumIncomingValues() ||<br class="">
----------------<br class="">
This should be named PhiNum according to the llvm convensions<br class="">
<br class="">
================<br class="">
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1824-1826<br class="">
@@ +1823,5 @@<br class="">
+ return false;<br class="">
+ for (unsigned i = 0; i < phiNum; i++)<br class="">
+ if (PhiCurr->getIncomingValue(i) != PhiBase->getIncomingValue(i))<br class="">
+ return false;<br class="">
+ return true;<br class="">
----------------<br class="">
I think you should check that incoming blocks are equivalent. In theory we can have two phi nodes with similar incoming values which are assigned to the different basic blocks, i.e:<br class="">
base_value = phi (%a, BB1), (%b, BB2)<br class="">
current_value = phi (%a, BB2), (%b, BB1)<br class="">
<br class="">
Also we can have two equivalent phi nodes with values recorder in a different order, i.e:<br class="">
base_value = phi (%a, BB1), (%b, BB2)<br class="">
current_value = phi (%b, BB2), (%a, BB1)<br class="">
<br class="">
<br class="">
<br class="">
<a href="https://reviews.llvm.org/D23920" class="">https://reviews.llvm.org/D23920</a><br class="">
<br class="">
<br class="">
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</body>
</html>