<span>Maybe. It may not fix it directly because you never use %1 or %2 again. I haven't looked to see how good the lookup is.</span><div><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 10, 2016, 3:45 PM Josh Klontz <<a href="mailto:josh.klontz@gmail.com">josh.klontz@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thanks Daniel, with that knowledge I think I can at least work around the issue in my frontend.<div><br></div><div>Ignoring GVN for a second though, and just looking at Early CSE, it seems to me that at least in this pass that there is the potential for an easy fix. Here the issue appears to be that we hit</div><div><br></div><div> if (Value *V = SimplifyInstruction(Inst, DL, &TLI, &DT, &AC))</div><div><br></div><div>immediately replacing %1 with 3 before we even reach %3. If we were to record this replacement in EarlyCSE::AvailableValues, wouldn't that address the issue? I'll try this out and see.</div><div><br></div><div>v/r,</div><div>Josh</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 10, 2016 at 4:23 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Yeah, that change is completely unrelated, that is about correctness, this is about optimization.<div>I'm working on a proposal to just fix assume at some point to deal with the former issue.</div><div><br></div><div>The problem with this testcase is that all the ways assume is propagate expect the variable in the assume to later be used.</div><div><br></div><div><div><br></div><div><This is the main way assume constants are propagated><br>bool GVN::processAssumeIntrinsic(IntrinsicInst *Inst) {</div><div><div>...</div><div><br></div><div> // We can replace assume value with true, which covers cases like this:</div><div> // call void @llvm.assume(i1 %cmp)</div><div> // br i1 %cmp, label %bb1, label %bb2 ; will change %cmp to true</div><div> ReplaceWithConstMap[V] = True;</div></div><div>...</div><div>}</div></div><div><br></div><div>...</div><div>bool GVN::processBlock(BasicBlock *BB) {<br></div><div>...</div><div><div> // Clearing map before every BB because it can be used only for single BB.</div><div> ReplaceWithConstMap.clear();</div></div><div>....</div><div><br></div><div>}</div><div><br></div><div><br></div><div>So it's going to go through the rest of the bb, see nothing with %2, do nothing, and then next iteration, clear the constant map. It's not valid to avoid clearing the constant map, and in fact, what is happening here is a pretty complicated to help with.</div><div>There is no easy way to see that the load at %3 is affected at all by the assume.</div><div><br></div><div>It's possible to make this work using the predication/value inference algorithms in the paper newgvn is based on, but it's not even implemented there.</div><div> <br></div><div>Short answer, without special casing this in magic ways, i wouldn't expect this to get fixed anytime soon.</div><div><br></div><div>If we fixed assume in one of the ways i thought about, like bodik's extended ssa:</div><div><a href="http://homepages.dcc.ufmg.br/~fernando/classes/dcc888/ementa/slides/RangeAnalysis.pdf" target="_blank">http://homepages.dcc.ufmg.br/~fernando/classes/dcc888/ementa/slides/RangeAnalysis.pdf</a></div><div><br></div><div>You would at least see that the load result is used by an assume, and could go look at that assume and so something with it. Currently, it's a few steps away.</div><div><br></div><div>In the current scheme, assumes just float in air, and so it can be hard to see what their effects touch</div><div>:)</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On Fri, Jun 10, 2016 at 2:00 PM, Josh Klontz via llvm-dev <span dir="ltr"><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div dir="ltr">Thanks for the lead Kevin. Unfortunately when I updated to ToT the problem persists. Will put together a minimum reproducing example.</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 10, 2016 at 12:26 PM, Smith, Kevin B <span dir="ltr"><<a href="mailto:kevin.b.smith@intel.com" target="_blank">kevin.b.smith@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">You might look at this commit to fix the problem: r270823<br>
"MemorySSA: Revert r269678 and r268068; replace with special casing in MemorySSA." <br>
<br>
I think that might fix the issue for you.<br>
<span><font color="#888888"><br>
Kevin Smith<br>
<br>
</font></span></blockquote></div><br></div>
<br></div></div>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
<br></blockquote></div><br></div>
</blockquote></div><br></div>
</blockquote></div></div>