[InstCombine] Don't limit operands in eraseInstFromFunction()

[nikic]

Summary

eraseInstFromFunction() adds the operands of the erased instructions, as those might now be dead as well. However, this is limited to instructions with less than 8 operands.

This check doesn't make a lot of sense to me. As the instruction gets removed afterwards, I don't see a potential for anything overly pathological happening here (as we can only add those operands to the worklist once). The impact on CTMark is in the noise. We also have the same code in instruction sinking and don't limit the operand count there.

[lebedev.ri]

This blames all the way back to rL80483:

inline the trivial AddToWorkList/RemoveFromWorkList methods into their callers. simplify ReplaceInstUsesWith. Make EraseInstFromFunction only add operands to the worklist if there aren't too many of them (this was a scalability win for crazy programs that was only infrequently enforced). Switch more code to using EraseInstFromFunction instead of duplicating it inline. Change some fcmp/icmp optimizations to modify fcmp/icmp in place instead of creating a new one and deleting the old one just to change the predicate.

So much like waymarking, some optimization that doesn't have clear consequences.
I think this shouldn't be here, at least for the consistency reasons, unless there's a testcase that shows otherwise.

[nikic]

In D77325#1961451, @lebedev.ri wrote:

This blames all the way back to rL80483:

inline the trivial AddToWorkList/RemoveFromWorkList methods into their callers. simplify ReplaceInstUsesWith. Make EraseInstFromFunction only add operands to the worklist if there aren't too many of them (this was a scalability win for crazy programs that was only infrequently enforced). Switch more code to using EraseInstFromFunction instead of duplicating it inline. Change some fcmp/icmp optimizations to modify fcmp/icmp in place instead of creating a new one and deleting the old one just to change the predicate.

So much like waymarking, some optimization that doesn't have clear consequences.
I think this shouldn't be here, at least for the consistency reasons, unless there's a testcase that shows otherwise.

Thanks for tracking down where this came from!

If this does turn out to cause performance issues, we should be able to mitigate this in a more principled fashion, by distinguishing things that are added to the worklist for (primarily) DCE purposes.

lib/Transforms/InstCombine/InstCombineInternal.h

public:
   /// methods should return the value returned by this function.
   Instruction *eraseInstFromFunction(Instruction &I) {
     LLVM_DEBUG(dbgs() << "IC: ERASE " << I << '\n');
     assert(I.use_empty() && "Cannot erase instruction that is used!");
     salvageDebugInfoOrMarkUndef(I);
 
     // Make sure that we reprocess all operands now that we reduced their
     // use counts.
-    if (I.getNumOperands() < 8) {
-      for (Use &Operand : I.operands())
-        if (auto *Inst = dyn_cast<Instruction>(Operand))
-          Worklist.add(Inst);
-    }
+
     Worklist.remove(&I);
     I.eraseFromParent();
     MadeIRChange = true;
     return nullptr; // Don't do anything with FI
   }
 
   void computeKnownBits(const Value *V, KnownBits &Known,
                         unsigned Depth, const Instruction *CxtI) const {