On 20 January 2011 11:23, Dan Gohman <span dir="ltr"><<a href="mailto:gohman@apple.com">gohman@apple.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

<div class="im"><br>
On Jan 19, 2011, at 2:03 PM, Nick Lewycky wrote:<br>
<br>
> On 19 January 2011 13:01, Dan Gohman <<a href="mailto:gohman@apple.com">gohman@apple.com</a>> wrote:<br>
><br>
> On Jan 18, 2011, at 12:32 AM, Nick Lewycky wrote:<br>
><br>
> > Hi,<br>
> ><br>
> > I tracked down a bug in indvars where we weren't updating SCEV properly. The attached patch shows the fix to this bug with a testcase, but it also causes five new test failures.<br>
><br>
> Indvars isn't restructuring the loop there, so it ideally shouldn't<br>
> need to call forgetLoop().<br>
><br>
> Hmm? It certainly is, it's changing the condition on the branch instruction that makes up the loop latch!<br>
><br>
> Offhand, is it possible that the problem<br>
> here is the same as the one in PR8037?<br>
><br>
> No, that only shows up when you try to run "opt -analyze -indvars -scalar-evolution" by causing a crash. With that patch in place, you can see that "opt -analyze -indvars -scalar-evolution" produces different values for the SCEVs than you get with "opt -indvars | opt -analyze -scalar-evolution" which led me to add the forgetLoop here.<br>


<br>
</div>Ah, I tried the testcase, saw the crash, and assumed that was the bug<br>
you were aiming to fix. The forgetLoop() call shouldn't be necessary<br>
for correctness.</blockquote><div><br></div><div>Why not though? Is there some guarantee that the values of the variables within the loop can't have been changed by this? I expected to find such a property based on my understanding of indvars, but when I looked through the code I couldn't convince myself of it. Can you make such an argument?</div>

<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> But I do see how ScalarEvolution's cached tripcounts<br>
are sub-optimal in this case, because they retain the type of the loop's<br>
original induction variable, rather than the type that indvars has<br>
chosen for the new induction variable. So forgetLoop() makes sense<br>
there.<br></blockquote><div><br></div><div>That's what I expected, and what I see in the testcase attached. However, a bunch of tests fail with this patch in:</div><div><br></div><div><blockquote class="gmail_quote" style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; ">

Failing Tests (5):<br>    LLVM :: Transforms/IndVarSimplify/ashr-tripcount.ll<br>    LLVM :: Transforms/IndVarSimplify/iv-sext.ll<br>    LLVM :: Transforms/IndVarSimplify/preserve-signed-wrap.ll<br>    LLVM :: Transforms/IndVarSimplify/promote-iv-to-eliminate-casts.ll<br>

    LLVM :: Transforms/IndVarSimplify/signed-trip-count.ll</blockquote></div><div><br></div><div>and as far as I can see, they're not just silly errors because we changed the SCEVs slightly; we're actually generating worse code when with forgetLoop() in indvars. That's what I don't understand.</div>

<div><br></div><div>I debugged one example and that led to the simplification I added to SCEV in r123838, but we're still not eliminating the sext instructions that the testcases expect.</div><meta http-equiv="content-type" content="text/html; charset=utf-8"><div>

<br></div><div>Nick</div><div><br></div></div>