[InstCombine] When rewriting operands in SimplifyAssociativeOrCommutative, add the original nodes back to the worklist so they'll get eagerly DCEd

[craig.topper]

Summary

If the original operands become dead they'll stick around until the next iteration of the InstCombine worklist. The worklist builder for that iteration will DCE the last dead node. But if that node was the only thing keeping an earlier node alive, the earlier node will have already been added to the worklist. So earlier nodes won't be DCEd until they are popped from the worklist. Any DCE from the worklist will be counted as a modification that will trigger another iteration of InstCombine even if nothing else was changed.

This patch explicitly adds the nodes back to the worklist so they will be DCEd in the worklist iteration they become dead. The worklist DCE code should take care of adding dead operands to the worklist so the process will remove all dead nodes. This will prevent the worklist builder for the next iteration from having any dead nodes from this. So we can stop another InstCombine iteration from being triggered.

There other places that use setOperand that are probably subject to this same problem. I know we've tried to rewrite operands to save allocations.

[spatel]

Is it possible to show the difference using debug output? If not, I think we should at least have a (small?) example here in the review, so there's some record of the problem for future reference.

[craig.topper]

Need to find the test case I observed this on originally.

lib/Transforms/InstCombine/InstructionCombining.cpp

if (I.isAssociative()) {
             // Note: this is only valid because SimplifyBinOp doesn't look at
             // the operands to Op0.
             I.clearSubclassOptionalData();
             I.setHasNoSignedWrap(true);
           } else {
             ClearSubclassDataAfterReassociation(I);
           }
 
+          Worklist.Add(Op0);
+          if (auto *CI = dyn_cast<Instruction>(C))
+            Worklist.Add(CI);
           Changed = true;
           ++NumReassoc;
           continue;
         }
       }
 
       // Transform: "A op (B op C)" ==> "(A op B) op C" if "A op B" simplifies.
       if (Op1 && Op1->getOpcode() == Opcode) {
         Value *A = I.getOperand(0);
         Value *B = Op1->getOperand(0);
         Value *C = Op1->getOperand(1);
 
         // Does "A op B" simplify?
         if (Value *V = SimplifyBinOp(Opcode, A, B, SQ.getWithInstruction(&I))) {
           // It simplifies to V.  Form "V op C".
           I.setOperand(0, V);
           I.setOperand(1, C);
           // Conservatively clear the optional flags, since they may not be
           // preserved by the reassociation.
           ClearSubclassDataAfterReassociation(I);
+          if (auto *AI = dyn_cast<Instruction>(A))
+            Worklist.Add(AI);
+          Worklist.Add(Op1);
           Changed = true;
           ++NumReassoc;
           continue;
         }
       }
     }
 
     if (I.isAssociative() && I.isCommutative()) {
if (I.isAssociative() && I.isCommutative()) {
         // Does "C op A" simplify?
         if (Value *V = SimplifyBinOp(Opcode, C, A, SQ.getWithInstruction(&I))) {
           // It simplifies to V.  Form "V op B".
           I.setOperand(0, V);
           I.setOperand(1, B);
           // Conservatively clear the optional flags, since they may not be
           // preserved by the reassociation.
           ClearSubclassDataAfterReassociation(I);
+          Worklist.Add(Op0);
+          if (auto *CI = dyn_cast<Instruction>(C))
+            Worklist.Add(CI);
           Changed = true;
           ++NumReassoc;
           continue;
         }
       }
 
       // Transform: "A op (B op C)" ==> "B op (C op A)" if "C op A" simplifies.
       if (Op1 && Op1->getOpcode() == Opcode) {
         Value *A = I.getOperand(0);
         Value *B = Op1->getOperand(0);
         Value *C = Op1->getOperand(1);
 
         // Does "C op A" simplify?
         if (Value *V = SimplifyBinOp(Opcode, C, A, SQ.getWithInstruction(&I))) {
           // It simplifies to V.  Form "B op V".
           I.setOperand(0, B);
           I.setOperand(1, V);
           // Conservatively clear the optional flags, since they may not be
           // preserved by the reassociation.
           ClearSubclassDataAfterReassociation(I);
+          if (auto *AI = dyn_cast<Instruction>(A))
+            Worklist.Add(AI);
+          Worklist.Add(Op1);
           Changed = true;
           ++NumReassoc;
           continue;
         }
       }
 
       // Transform: "(A op C1) op (B op C2)" ==> "(A op B) op (C1 op C2)"
       // if C1 and C2 are constants.
if (I.isAssociative() && I.isCommutative()) {
         }
         InsertNewInstWith(NewBO, I);
         NewBO->takeName(Op1);
         I.setOperand(0, NewBO);
         I.setOperand(1, ConstantExpr::get(Opcode, C1, C2));
         // Conservatively clear the optional flags, since they may not be
         // preserved by the reassociation.
         ClearSubclassDataAfterReassociation(I);
+        Worklist.Add(Op0);
+        Worklist.Add(Op1);
 
         Changed = true;
         continue;
       }
     }
 
     // No further simplifications.
     return Changed;