[PATCH] D52508: [InstCombine] Clean up after IndVarSimplify

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

This looks good to me, one more nit.
I think we should deal with `do while` in another patch.



================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1202
+  if (match(&I, m_c_BinOp(m_Value(A), m_OneUse(m_Not(m_OneUse(SelectPred))))) &&
+      IsFreeToInvert(B, B->hasOneUse())) {
+    Value *NotB = Builder.CreateNot(B);
----------------
Why is there free-invert restriction on `B`?
We had 3 instructions:
1. `add A, not(select)`; root, will always get replaced
2. `not(select)`; does not exist in the new pattern, should go away, thus one-use
3. `Select(P, A, B)`;  does not exist in the new pattern, should go away, thus one-use
Now, we replace them with 3 instructions:
1. `Select(P, -1, (add A, not(B)))`
2. `add A, not(B)`
3. `not(B)` which is free since `B` is freely invertible.

But we had 3 instructions and produce 3 instructions, even if `B` is not free to invert.
So why restrict to freely-invertivbe `B`?
If it is indeed needed, could you please add it as a comment in the diff ^.


https://reviews.llvm.org/D52508