<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 17/07/17 15:48, Daniel Berlin wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF4BwTX2554Fe7pyPkPMZ-jEzAOaTnr147kjL81EpXV_eJb-pQ@mail.gmail.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Mon, Jul 17, 2017 at 2:04 AM,
Pedro Ferreira via Phabricator <span dir="ltr"><<a
href="mailto:reviews@reviews.llvm.org" target="_blank"
moz-do-not-send="true">reviews@reviews.llvm.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">PFerreira
created this revision.<br>
Herald added a reviewer: dberlin.<br>
<br>
Related to bugzilla 33731 <a
href="https://bugs.llvm.org/show_bug.cgi?id=33731"
rel="noreferrer" target="_blank" moz-do-not-send="true">https://bugs.llvm.org/show_<wbr>bug.cgi?id=33731</a><br>
<br>
This is caused by a a PRE insertion when the predecessor
block hasn't yet been through Value Numbering (due to a
back-edge).<br>
</blockquote>
<div><br>
</div>
<div>I'm not surprised this is buggy, sadly.</div>
<div>I suspect there are a bunch of issues around this kind
of thing. </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
The PRE insertion adds a duplicate instruction at the end
of the block, but the already-existing instruction is
already used in the predecessor block, like so:<br>
<br>
L.LB1_346: ; preds
= %L.LB1_423<br>
<br>
%7 = sext i32 %2 to i64<br>
%8 = mul i64 %7, 4<br>
<br>
</blockquote>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
- %.pre = sext i32 %1 to i64** ; inserted by
GVN::performScalarPREInsertion<br>
<br>
When we then iterate over this block (later in the pass),
GVN chooses to delete the first instance of the
instruction as it is duplicated (instead of the later one
which was created before).</blockquote>
<div>This sounds like an issue with
findLeader/addToLeaderTable not checking local dominance
if they are in the same block, based on the assumption
that it will process the block in order.</div>
</div>
</div>
</div>
</blockquote>
<br>
I agree; there is a comment in "performScalarPREInsertion" stating
exactly that.<br>
<br>
<blockquote type="cite"
cite="mid:CAF4BwTX2554Fe7pyPkPMZ-jEzAOaTnr147kjL81EpXV_eJb-pQ@mail.gmail.com">
<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>
I'm not sure I understand - the instruction in particular has
already been numbered on a different block. That is, there are
equivalent instructions in other blocks which DO get numbered, so
essentially CSE will eventually move the instruction to a common
dominator (or at least it should).<br>
That being said, the "%7 = sext i32 %2 to i64" already has a number.
The problem is that this particular instance has not been added to
the leader list and so PREInsertion adds one (since it assumes there
isn't one).<br>
<br>
<blockquote type="cite"
cite="mid:CAF4BwTX2554Fe7pyPkPMZ-jEzAOaTnr147kjL81EpXV_eJb-pQ@mail.gmail.com">
<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::<wbr>performScalarPREInsertion" 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>
But wouldn't that require that the instruction exists in the leader
list?<br>
If I do check the dominance, wouldn't I have to scan the incoming
block anyway?<br>
<br>
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="p" 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; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: pre-wrap; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;">(</span><span class="n" 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; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: pre-wrap; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;">DT</span><span class="o" 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; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: pre-wrap; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;">-></span><span class="n" 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; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: pre-wrap; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;">dominates</span><span class="p" 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; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: pre-wrap; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;">(</span><span class="n" 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; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: pre-wrap; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;">I</span><span class="p" 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; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: pre-wrap; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 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; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: pre-wrap; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 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="n" 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; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: pre-wrap; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px;">R</span><span class="p" 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; orphans: 2; text-align: left; text-indent: 0px; text-transform: none; white-space: pre-wrap; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 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).<br>
<br>
<br>
<blockquote type="cite"
cite="mid:CAF4BwTX2554Fe7pyPkPMZ-jEzAOaTnr147kjL81EpXV_eJb-pQ@mail.gmail.com">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
I though about stoping "GVN::<wbr>performScalarPREInsertion"
from inserting the instruction by numbering the
BasicBlock, but that might lead to infinite loops. I also
tried to stop PREInsertion from running if the BasicBlock
had not been numbered yet (by keeping a list of numbered
blocks) but that wouldn't work since GVN only does one
pass over the function (so this case would never be
optimised out).<br>
<br>
The patch does fix the bug though, without adding
regressions.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D35475" rel="noreferrer"
target="_blank" moz-do-not-send="true">https://reviews.llvm.org/<wbr>D35475</a><br>
<br>
Files:<br>
lib/Transforms/Scalar/GVN.cpp<br>
<br>
<br>
Index: lib/Transforms/Scalar/GVN.cpp<br>
==============================<wbr>==============================<wbr>=======<br>
--- lib/Transforms/Scalar/GVN.cpp<br>
+++ lib/Transforms/Scalar/GVN.cpp<br>
@@ -1807,6 +1807,13 @@<br>
return false;<br>
}<br>
<br>
+ // If this was a shortcut PRE, it will have been
inserted at the end<br>
+ // of the block; ensure that it dominates all uses of
the original.<br>
+ if (Instruction *R = dyn_cast<Instruction>(Repl))
{<br>
+ if (DT->dominates(I, R)) {<br>
+ R->moveBefore(I);<br>
+ }<br>
+ }<br>
// Remove it!<br>
patchAndReplaceAllUsesWith(I, Repl);<br>
if (MD && Repl->getType()-><wbr>isPtrOrPtrVectorTy())<br>
<br>
<br>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>