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

[David Majnemer]

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.
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.

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