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

Thu Feb 23 11:35:43 PST 2017

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