[llvm-commits] [llvm] r168711 - in /llvm/trunk: lib/Analysis/ScalarEvolution.cpp test/Transforms/IndVarSimplify/eliminate-comparison.ll

Benjamin Kramer benny.kra at gmail.com
Thu Nov 29 11:04:11 PST 2012


On 29.11.2012, at 19:33, Andrew Trick <atrick at apple.com> wrote:

> 
> On Nov 29, 2012, at 8:00 AM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>>>> 
>>>> Chris, can you approve this for 3.2? It fixes a miscompilation and is apparently a regression from 3.1.
>>>> 
>>>> - Ben
>>>> 
>>>>> 
>>>>> Modified:
>>>>> llvm/trunk/lib/Analysis/ScalarEvolution.cpp
>>>>> llvm/trunk/test/Transforms/IndVarSimplify/eliminate-comparison.ll
>>>>> 
>>>>> Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=168711&r1=168710&r2=168711&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
>>>>> +++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Tue Nov 27 12:16:32 2012
>>>>> @@ -6149,9 +6149,10 @@
>>>>> if (SimplifyICmpOperands(Pred, LHS, RHS))
>>>>>  if (LHS == RHS)
>>>>>    return CmpInst::isTrueWhenEqual(Pred);
>>>>> -  if (SimplifyICmpOperands(FoundPred, FoundLHS, FoundRHS))
>>>>> -    if (FoundLHS == FoundRHS)
>>>>> -      return CmpInst::isFalseWhenEqual(Pred);
>>>>> +
>>>>> +  // Canonicalize the found cond too.  We can't conclude a result from the
>>>>> +  // simplified values.
>>>>> +  SimplifyICmpOperands(FoundPred, FoundLHS, FoundRHS);
> 
> Hi Ben,
> 
> Thanks for the fix. But why didn't you go for the more obvious fix?
> 
> -      return CmpInst::isFalseWhenEqual(Pred);
> +      return CmpInst::isFalseWhenEqual(FoundPred);
> 
> As an example, invert the predicate in your test case:
> 
> %tmp76 = icmp ne i32 %sext34, %sext21
> 
> In this case, I think the following compare should be eliminated, but won't be with your fix:
> 
> %tmp46 = icmp slt i32 %__key8.0, 10
> 
> -Andy

Hah, sometimes you're too blind to see the obvious. I'll fix this, thanks for the review.

The original patch is still safe for 3.2, it only misses a potential optimization.

- Ben



More information about the llvm-commits mailing list