[PATCH] Reassociate: reprocess RedoInsts after each instruction

[Mehdi AMINI]

Hi David,

Let’s consider:

  %_0 = add i32 %in, 1
  %_1 = mul i32 %in, -2
  %_2 = add i32 %_0, %_1
  %_3 = add i32 %_1, %_2
  %_4 = add i32 %_3, 1
  %_5 = add i32 %in, %_3
  ret i32 %_5

Now when processing %3, the expression is:

  "%1 + %in + 1 + %1” 

and it is factorized to:

  “%in + 1 + ( 2 * %1)”

the IR becomes:

  %_0 = add i32 %in, 1
  %_1 = mul i32 %in, -2
  %factor = mul i32 %_1, 2
  %_2 = add i32 %in, 1
  %_3 = add i32 %_2, %factor
  %_4 = add i32 %_3, 1
  %_5 = add i32 %in, %_3
  ret i32 %_5

And %factor was added to the RedoInsts list because we known it might not be canonicalized.
And indeed in this case it is 2*(%1*-2) which can be turned into -4*%1.

Let continue to %5, it will consider

  %in + %in + 1 + %factor

It will try to factorize, and list for all of the operands the factors to find some in common. 
When processing %factor, the list of factors is

  %in * -2 * 2

However this contains two constant and is forbidden because it should have been folded earlier. Because RedoInsts was processed at the end of the block instead of at the end of a transformation this canonicalization would have happened later.

Does it makes sense?

Thanks.


http://reviews.llvm.org/D6995

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

Index: lib/Transforms/Scalar/Reassociate.cpp
===================================================================
--- lib/Transforms/Scalar/Reassociate.cpp
+++ lib/Transforms/Scalar/Reassociate.cpp
@@ -2264,22 +2264,26 @@
   MadeChange = false;
   for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE; ++BI) {
     // Optimize every instruction in the basic block.
-    for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II != IE; )
+    for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II != IE; ) {
       if (isInstructionTriviallyDead(II)) {
         EraseInst(II++);
       } else {
         OptimizeInst(II);
         assert(II->getParent() == BI && "Moved to a different block!");
         ++II;
       }
 
-    // If this produced extra instructions to optimize, handle them now.
-    while (!RedoInsts.empty()) {
-      Instruction *I = RedoInsts.pop_back_val();
-      if (isInstructionTriviallyDead(I))
-        EraseInst(I);
-      else
-        OptimizeInst(I);
+      // If this produced extra instructions to optimize, handle them now.
+      while (!RedoInsts.empty()) {
+        Instruction *I = RedoInsts.pop_back_val();
+        if (I == II)
+          // Will be processed next iteration on the basic block
+          continue;
+        if (isInstructionTriviallyDead(I))
+          EraseInst(I);
+        else
+          OptimizeInst(I);
+      }
     }
   }
 
Index: test/Transforms/Reassociate/crash3.ll
===================================================================
--- /dev/null
+++ test/Transforms/Reassociate/crash3.ll
@@ -0,0 +1,15 @@
+; RUN: opt -reassociate -o - -S %s | FileCheck %s
+
+declare void @bar(i32)
+
+define i32 @foo(i32 %in) {
+; CHECK-LABEL: @foo
+; CHECK: %_factor = mul i32 %_3, -4
+  %_0 = add i32 %in, 1
+  %_1 = mul i32 %in, -2
+  %_2 = add i32 %_0, %_1
+  %_3 = add i32 %_1, %_2
+  %_4 = add i32 %_3, 1
+  %_5 = add i32 %in, %_3
+  ret i32 %_5
+}

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D6995.18335.patch
Type: text/x-patch
Size: 1931 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150117/01caf57a/attachment.bin>