[PATCH] IndVarSimplify does not check if loop invariant expansion can trap

[Andrew Trick]

On Jun 4, 2013, at 1:29 PM, David Majnemer <david.majnemer at gmail.com> wrote:

> It isn't all divide operations. Just the ones that we can't prove have a non-zero denominator.
> 
> I think that the proper thing to do is to stick in in the loop preheader.

I don’t think we want to stick anything in the preheader in this case.

> However, many other pieces of code see that it is loop invariant and hoist it out of said loop preheader.
> I'll give it a shot later.

Yes, LICM takes care of hoisting loop invariant divides. Rafael showed that the case you disabled is not worth optimizing.

I think the only better fix here is to rewrite RewriteLoopExitValues so it does not depend on SCEVExpander (major undertaking). Trying to embed IR rewriting constraints inside SCEV and messing with insertion points is horrible. I’d prefer to just sink a chain of instructions and let IR utilities reduce the expression. If we want to hoist instructions to hide latency an MI pass should do that.

Thanks for the fix!

-Andy

> 
> -- 
> David Majnemer
> 
> 
> On Tue, Jun 4, 2013 at 8:48 AM, James Molloy <james at jamesmolloy.co.uk> wrote:
> > The only case this could regress is a potentially trapping operation that doesn't actually trap for reasons that are invisible to the optimizer.
> 
> Like a divide operation where the divisor never happens to be zero? or am I misunderstanding the patch?
> 
> My understanding is that now we will not hoist divides out of induction variable expressions where before we did, because divides can trap. Most divides in user code don't trap, so I don't see how you can conclude that the optimization is not firing on user code.
> 
> That said, I agree let's fix the crash with the trivial fix proposed and deal with the performance impact later.
> 
> On 4 June 2013 16:41, Rafael Ávila De Espíndola <rafael.espindola at gmail.com> wrote:
> We are more likely to fix a crash. So I don't think it is firing. The only case this could regress is a potentially trapping operation that doesn't actually trap for reasons that are invisible to the optimizer.
> 
> Adding the extra control flow to guard it is not clear to be a win. We should probably fix the miscompilation first. If we do have a perf regression somewhere, it can be addressed in a subsequent patch.
> 
> Sent from my iPhone
> 
> On 2013-06-04, at 11:22, James Molloy <james at jamesmolloy.co.uk> wrote:
> 
>> Hi Rafael,
>> 
>> I don't know. Do we know this broken optimization doesn't kick in in the test suite? If it does, we'd regress on performance if we took the conservative way out.
>> 
>> Just my 2c.
>> 
>> Cheers,
>> 
>> James
>> 
>> On 4 June 2013 15:40, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>> On 4 June 2013 09:49, James Molloy <james at jamesmolloy.co.uk> wrote:
>> > Hi David,
>> >
>> > Might a more performant solution not be to wrap any hoisted instructions
>> > that are not safe to expand in a conditional that tests if the loop will be
>> > entered at least once?
>> 
>> Is that really worth it? Given that this bug went undetected for years
>> (it was already broken in r122610), I don't think it would kick often
>> enough to justify it.
>> 
>> Cheers,
>> Rafael
>> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130605/2b4efa70/attachment.html>