[PATCH] D52177: [InstCombine] Fold ~A - Min/Max(~A, O) -> Max/Min(A, ~O) - A

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

In https://reviews.llvm.org/D52177#1244827, @spatel wrote:

> I was hoping to find a more general solution for min/max with nots, but I'm not seeing it, so just a few nits in the inline comments.


I have another I'm afraid. Over in https://reviews.llvm.org/D52508.

> Craig's been fighting infinite loops in this area, so let's see if he has any comments on the safety constraints.

Any suggestions on finding these? I've run the testsuite and a bootstrap, I presume that wouldn't catch much?



================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:181-183
+  // Selects with invertable operands are freely invertable
+  if (match(V, m_Select(m_Value(), m_Not(m_Value()), m_Not(m_Value()))))
+    return WillInvertAllUses;
----------------
spatel wrote:
> This is similar to what I was imagining in a comment in D51964, but I'm still not sure if we need to special-case min/max patterns for the extra-uses.
> 
> The LHS->hasNUsesOrMore(3) hack in the caller might be enough...
The LHS->hasNUsesOrMore(3) isn't _meant_ as a hack. Unless you mean it's ugly? It should have 2 uses from the min/max, so 3+ means we wouldn't invert all uses.

The motivating case here (umin3_not_all_ops_extra_uses_invert_subs) is something like min(min(not(a), not(b)), not(c)), but with subs in there too. So there's two min's, one we are folding from, and other we are checking is freely invertible. That's what this is trying to catch.

I was reluctant to make IsFreeToInvert recursive, but that would make it more powerful and remove the need for just checking m_Not's here.


https://reviews.llvm.org/D52177