[llvm] [InstCombine] Improve handling of `not` and free inversion. (PR #66787)

[Nikita Popov via llvm-commits]

================
@@ -2198,12 +2198,14 @@ Instruction *InstCombinerImpl::visitSub(BinaryOperator &I) {
   // (~X) - (~Y) --> Y - X
   // This is placed after the other reassociations and explicitly excludes a
   // sub-of-sub pattern to avoid infinite looping.
-  if (isFreeToInvert(Op0, Op0->hasOneUse()) &&
-      isFreeToInvert(Op1, Op1->hasOneUse()) &&
-      !match(Op0, m_Sub(m_ImmConstant(), m_Value()))) {
-    Value *NotOp0 = Builder.CreateNot(Op0);
-    Value *NotOp1 = Builder.CreateNot(Op1);
-    return BinaryOperator::CreateSub(NotOp1, NotOp0);
+  if (!match(Op0, m_Sub(m_ImmConstant(), m_Value())) &&
+      isFreeToInvert(Op0, Op0->hasOneUse())) {
+    if (Value *NotOp1 = getFreelyInverted(Op1, Op1->hasOneUse(), &Builder)) {
+      Value *NotOp0 = getFreelyInverted(Op0, Op0->hasOneUse(), &Builder);
+      assert(NotOp0 != nullptr &&
+             "isFreeToInvert desynced with getFreelyInverted");
+      return BinaryOperator::CreateSub(NotOp1, NotOp0);
+    }
----------------
nikic wrote:

I strongly suspect that this fold is going to bite us now that we do the recursive walk. We can go back and forth between `(~X) - (~Y)` and `Y - X` as long as all of `X`, `Y`, `~X`, `~Y` are freely invertible.

Previously this would happen with constants (which this guards against), but I think now it can likely happen in more complex cases that root out at constants as well. I'm thinking something like `sub (select x, C1, C2), (select y, C3, C4)` might infinite loop, depending on whether the "binop of select" fold happens before this one or not. Or if not that, then maybe something like that with more operations in between.

Basically, for this transform it is important that we actually remove at least one instruction, not just "freely" invert in the sense that instruction count stays the same.

https://github.com/llvm/llvm-project/pull/66787