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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

> I think we should deal with do while in another patch.

Yeah, defo. I just need to come up with a sensible way to fix it. I feel some of this is pushing against the edges of what instcombine should be doing, but I'll keep pushing until someone tells me to stop.



================
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);
----------------
lebedev.ri wrote:
> 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 ^.
Sure, sounds about right. I was going for "makes things better" as opposed to "doesn't make things worse".

Consider the code:
  %l0 = icmp sgt i32 %A, %B
  %l1 = select i1 %l0, i32 %A, i32 %B
  %not = xor i32 %l1, -1
  %x = add i32 %not, %A
which is add(A,  not(max(A, B))
and we (would) convert it to select(icmp(a>b), -1, A+~B). On the target I was looking at we don't tend to have max instructions, but that's obviously not universal. At least if B is invertible we can say this looks better even without the max. (I mean, materialising -1 isn't always free, but that's perhaps more me thinking about inlining than about inst combine.)

Does that sound plausible enough? I've added a comment. If you disagree, I can change things the other way. I don't have benchmarks that suggest that either one is better than the other.


https://reviews.llvm.org/D52508