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

[via llvm-commits]

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