[PATCH] D30228: [Reassociate] Add negated value of negative constant to the Duplicates list.

[Chad Rosier via Phabricator via llvm-commits]

mcrosier created this revision.

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, 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 patch, which causes the assert reported in PR30256.

Chad


https://reviews.llvm.org/D30228

Files:
  lib/Transforms/Scalar/Reassociate.cpp
  test/Transforms/Reassociate/basictest.ll


Index: test/Transforms/Reassociate/basictest.ll
===================================================================
--- test/Transforms/Reassociate/basictest.ll
+++ test/Transforms/Reassociate/basictest.ll
@@ -222,3 +222,19 @@
 ; CHECK-LABEL: @test15
 ; CHECK: and i1 %A, %B
 }
+
+; PR30256 - Just make sure we don't hit an assert.
+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
+}
Index: lib/Transforms/Scalar/Reassociate.cpp
===================================================================
--- lib/Transforms/Scalar/Reassociate.cpp
+++ lib/Transforms/Scalar/Reassociate.cpp
@@ -1520,8 +1520,8 @@
       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 @@
           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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D30228.89280.patch
Type: text/x-patch
Size: 1914 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170221/abcfe586/attachment.bin>