[llvm] r296003 - [Reassociate] Add negated value of negative constant to the Duplicates list.

[via llvm-commits]

SGTM!  Thanks, Hans.

On 2017-02-23 14:35, Hans Wennborg wrote:
> I think the code review is as good an approval as we'll get.
> 
> Let's see how the buildbots like this, and I'll merge if it looks good
> after a day or two.
> 
> Thanks,
> Hans
> 
> On Thu, Feb 23, 2017 at 11:24 AM, via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>> Hi Hans,
>> This commit should resolve PR30256, which is a 4.0 release blocker.  
>> I'm not
>> exactly sure who should approve the cherry pick, however.
>> 
>>  Chad
>> 
>> 
>> On 2017-02-23 13:49, Chad Rosier via llvm-commits wrote:
>>> 
>>> Author: mcrosier
>>> Date: Thu Feb 23 12:49:03 2017
>>> New Revision: 296003
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=296003&view=rev
>>> Log:
>>> [Reassociate] Add negated value of negative constant to the 
>>> Duplicates
>>> list.
>>> 
>>> In OptimizeAdd, we scan the operand list to see if there are any 
>>> common
>>> factors
>>> between operands that can be factored out to reduce the number of
>>> multiplies
>>> (e.g., 'A*A+A*B*C+D' -> 'A*(A+B*C)+D'). For each operand of the 
>>> operand
>>> list, we
>>> only consider unique factors (which is tracked by the Duplicate set). 
>>> Now
>>> if we
>>> find a factor that is a negative constant, we add the negated value 
>>> as a
>>> factor
>>> as well, because we can percolate the negate out. However, we 
>>> mistakenly
>>> don't
>>> add this negated constant to the Duplicates set.
>>> 
>>> Consider the expression A*2*-2 + B. Obviously, nothing to factor.
>>> 
>>> For the added value A*2*-2 we over count 2 as a factor without this
>>> change,
>>> which causes the assert reported in PR30256.  The problem is that 
>>> this
>>> code is
>>> assuming that all the multiply operands of the add are already
>>> reassociated.
>>> This change avoids the issue by making OptimizeAdd tolerate 
>>> multiplies
>>> which
>>> haven't been completely optimized; this sort of works, but we're 
>>> doing
>>> wasted
>>> work: we'll end up revisiting the add later anyway.
>>> 
>>> Another possible approach would be to enforce RPO iteration order 
>>> more
>>> strongly.
>>> If we have RedoInsts, we process them immediately in RPO order, 
>>> rather
>>> than
>>> waiting until we've finished processing the whole function. 
>>> Intuitively,
>>> it
>>> seems like the natural approach: reassociation works on expression 
>>> trees,
>>> so
>>> the optimization only works in one direction. That said, I'm not sure 
>>> how
>>> practical that is given the current Reassociate; the "optimal" form 
>>> for an
>>> expression depends on its use list (see all the uses of 
>>> "user_back()"), so
>>> Reassociate is really an iterative optimization of sorts, so any 
>>> changes
>>> here
>>> would probably get messy.
>>> 
>>> PR30256
>>> 
>>> Differential Revision: https://reviews.llvm.org/D30228
>>> 
>>> Modified:
>>>     llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp
>>>     llvm/trunk/test/Transforms/Reassociate/basictest.ll
>>> 
>>> Modified: llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp
>>> URL:
>>> 
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp?rev=296003&r1=296002&r2=296003&view=diff
>>> 
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp Thu Feb 23 
>>> 12:49:03
>>> 2017
>>> @@ -1520,8 +1520,8 @@ Value *ReassociatePass::OptimizeAdd(Inst
>>>        if (ConstantInt *CI = dyn_cast<ConstantInt>(Factor)) {
>>>          if (CI->isNegative() && !CI->isMinValue(true)) {
>>>            Factor = ConstantInt::get(CI->getContext(), 
>>> -CI->getValue());
>>> -          assert(!Duplicates.count(Factor) &&
>>> -                 "Shouldn't have two constant factors, missed a
>>> canonicalize");
>>> +          if (!Duplicates.insert(Factor).second)
>>> +            continue;
>>>            unsigned Occ = ++FactorOccurrences[Factor];
>>>            if (Occ > MaxOcc) {
>>>              MaxOcc = Occ;
>>> @@ -1533,8 +1533,8 @@ Value *ReassociatePass::OptimizeAdd(Inst
>>>            APFloat F(CF->getValueAPF());
>>>            F.changeSign();
>>>            Factor = ConstantFP::get(CF->getContext(), F);
>>> -          assert(!Duplicates.count(Factor) &&
>>> -                 "Shouldn't have two constant factors, missed a
>>> canonicalize");
>>> +          if (!Duplicates.insert(Factor).second)
>>> +            continue;
>>>            unsigned Occ = ++FactorOccurrences[Factor];
>>>            if (Occ > MaxOcc) {
>>>              MaxOcc = Occ;
>>> 
>>> Modified: llvm/trunk/test/Transforms/Reassociate/basictest.ll
>>> URL:
>>> 
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Reassociate/basictest.ll?rev=296003&r1=296002&r2=296003&view=diff
>>> 
>>> ==============================================================================
>>> --- llvm/trunk/test/Transforms/Reassociate/basictest.ll (original)
>>> +++ llvm/trunk/test/Transforms/Reassociate/basictest.ll Thu Feb 23
>>> 12:49:03 2017
>>> @@ -222,3 +222,23 @@ define i32 @test15(i32 %X1, i32 %X2, i32
>>>  ; CHECK-LABEL: @test15
>>>  ; CHECK: and i1 %A, %B
>>>  }
>>> +
>>> +; PR30256 - previously this asserted.
>>> +; CHECK-LABEL: @test16
>>> +; CHECK: %[[FACTOR:.*]] = mul i64 %a, -4
>>> +; CHECK-NEXT: %[[RES:.*]] = add i64 %[[FACTOR]], %b
>>> +; CHECK-NEXT: ret i64 %[[RES]]
>>> +define i64 @test16(i1 %cmp, i64 %a, i64 %b) {
>>> +entry:
>>> +  %shl = shl i64 %a, 1
>>> +  %shl.neg = sub i64 0, %shl
>>> +  br i1 %cmp, label %if.then, label %if.end
>>> +
>>> +if.then:                                          ; preds = %entry
>>> +  %add1 = add i64 %shl.neg, %shl.neg
>>> +  %add2 = add i64 %add1, %b
>>> +  ret i64 %add2
>>> +
>>> +if.end:                                           ; preds = %entry
>>> +  ret i64 0
>>> +}
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits