[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:
> > 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).
> > 
> Sorry, but I'm not sure if I exactly got your point. Do you mean, we should remove the IsFreeToInvert anyway, because it's the backend's job to turn it back into whatever's best for it?
> 
> We have a few places where we don't do optimisations on min/max's in instcombine, I think under the assumption that if we break them apart it can be hard to put them back together, but I'm not sure. I'm happy to take it out. Like I said, I ran some benchmarks and couldn't see a difference either way (but perhaps wasn't using the best target for that).
I'm not sure. I'm only saying that avoiding some transform just because it may hurt backends
is not *always* the right choice. @spatel will know best.

See e.g. D46760, which is actually a https://bugs.llvm.org/show_bug.cgi?id=37872,
which we are not doing because https://reviews.llvm.org/rL155136 / https://reviews.llvm.org/rL155136
so it is now waiting for funnel shift support (https://bugs.llvm.org/show_bug.cgi?id=38152)
And if instead of simply not doing that opt in middle-end, the back-end would have been fixed back then,
how far further would we be now? </rant>

Anyway, if we keep this one-use restriction,
i *think* we also need to make sure that we have the *opposite* transform.
I.e.  `select(icmp(a>b), -1, A+~B)`  ->  `add(A, not(max(A, B))`  iff B is not free to invert.


https://reviews.llvm.org/D52508