<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 class="">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">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">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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org">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>