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

Fri Feb 24 11:05:46 PST 2017

Merged to 4.0 in r296156.

On Thu, Feb 23, 2017 at 11:50 AM,  <mcrosier at codeaurora.org> wrote:
> 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