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

[Dave Green via Phabricator via llvm-commits]

dmgreen 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);
----------------
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).


https://reviews.llvm.org/D52508