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

[James Molloy]

> 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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130604/7983fc93/attachment.html>