<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I replied on the review, if you stare at the last paragraph, i'm very confused how this bug could ever occur at all.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><span class="">
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>addToLeaderTable always adds to the end, and so it will
hit the PRE leader before any other.</div>
<div><br>
</div>
<div>If so, the fastest fix i can think of is to number the
instructions in the block starting at 0 as we process
them. An instruction can only be a leader when BB =
LeaderBB iff the number is < than the current
instruction's number.</div>
<div>Assign all PRE'd instructions numbers that are MAX_INT.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br></span>
I'm not sure I understand - the instruction in particular has
already been numbered on a different block.</div></blockquote><div>By numbering above, i mean literally assign them numbers starting at 0. IE DFS number them.</div><div>Not value number them.</div><div>See what NewGVN does with local numbers when computing the elimination order for instructions,.<br></div><div><br></div><div>The dominance ordering of instructions is completely described by <order of blocks in dominator tree, order of instructions in block>. Numbering them as you see them provides <order in block>.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><span class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>If you have two MAX_INT instructions, they are in-order
already (since PRE will only add them in order).</div>
<div><br>
</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> This is
because "GVN::performScalarPREInsertio<wbr>n" will number
the instruction as it is created (so it is in the
leader-list). </blockquote>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The attached patch moves the "%.pre" instruction up so
that the replacement is valid.<br>
<br>
I am not sure this is a valid fix, but I couldn't figure
out another solution.<br>
</blockquote>
<div>There are a couple other ways to fix this.</div>
<div>Another would be "always check local dominance using
OrderedInstruction in FindLeader ". This is equivalent to
the above. <br>
</div>
</div>
</div>
</div>
</blockquote>
<br></span>
But wouldn't that require that the instruction exists in the leader
list?<br></div></blockquote><div>No.</div><div>You'd do findLeader(CurInst), and compare local dominance of the leader and curinst. Leader can't be a leader if the number you gave it is > number for curinst.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
If I do check the dominance, wouldn't I have to scan the incoming
block anyway?<br>
<br></div></blockquote><div>No, that's why you number them above.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
I do have another alternative; just before where I added the
instruction moving, we have:<br>
<br>
Value *Repl = findLeader(I->getParent(), Num);<br>
if (!Repl) {<br>
// Failure, just remember this instance for future use.<br>
addToLeaderTable(Num, I, I->getParent());<br>
return false;<br>
} else if (Repl == I) {<br>
// If I was the result of a shortcut PRE, it might already be in
the table<br>
// and the best replacement for itself. Nothing to do.<br>
return false;<br>
}<br>
<br>
we could append to this if-else block:<br>
<br>
else if <span class="m_6613496951783309869p" style="text-decoration-line:none!important;text-decoration-style:initial;text-decoration-color:initial;display:inline;word-break:break-all;color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:11px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:left;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0px">(</span><span class="m_6613496951783309869n" style="text-decoration-line:none!important;text-decoration-style:initial;text-decoration-color:initial;display:inline;word-break:break-all;color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:11px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:left;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0px">DT</span><span class="m_6613496951783309869o" style="text-decoration-line:none!important;text-decoration-style:initial;text-decoration-color:initial;display:inline;word-break:break-all;color:rgb(170,34,17);font-family:Menlo,Consolas,Monaco,monospace;font-size:11px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:left;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0px">-></span><span class="m_6613496951783309869n" style="text-decoration-line:none!important;text-decoration-style:initial;text-decoration-color:initial;display:inline;word-break:break-all;color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:11px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:left;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0px">dominates</span><span class="m_6613496951783309869p" style="text-decoration-line:none!important;text-decoration-style:initial;text-decoration-color:initial;display:inline;word-break:break-all;color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:11px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:left;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0px">(</span><span class="m_6613496951783309869n" style="text-decoration-line:none!important;text-decoration-style:initial;text-decoration-color:initial;display:inline;word-break:break-all;color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:11px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:left;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0px">I</span><span class="m_6613496951783309869p" style="text-decoration-line:none!important;text-decoration-style:initial;text-decoration-color:initial;display:inline;word-break:break-all;color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:11px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:left;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0px">,</span><span style="color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:11px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:left;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0px;background-color:rgba(151,234,151,0.6);text-decoration-style:initial;text-decoration-color:initial;display:inline!important;float:none"> </span><span class="m_6613496951783309869n" style="text-decoration-line:none!important;text-decoration-style:initial;text-decoration-color:initial;display:inline;word-break:break-all;color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:11px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:left;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0px">R</span><span class="m_6613496951783309869p" style="text-decoration-line:none!important;text-decoration-style:initial;text-decoration-color:initial;display:inline;word-break:break-all;color:rgb(0,0,0);font-family:Menlo,Consolas,Monaco,monospace;font-size:11px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:left;text-indent:0px;text-transform:none;white-space:pre-wrap;word-spacing:0px">)) </span>{<br>
// Replace the existing leader since this one dominates it.<br>
replaceOnLeaderTable(Num, Repl, I);<br>
}<br>
<br>
This way we'd preserve the existing instruction (instead of
replacing it with the "%.pre" one).</div></blockquote><div><br></div><div>This seems like a hack :)<br><br></div></div></div></div>