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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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);
----------------
dmgreen wrote:
> 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.
Obvious counter-point - and what if you already have that bad pattern, which is suboptimal for the target?
If something isn't optimal for the target, then that should be optimized in the backend,
instead of handicapping middle-end. (also, there are more than one target).



https://reviews.llvm.org/D52508