[PATCH] D39960: [InstCombine] In foldSelectOpOp reuse existing selects if present

[Daniel Berlin via llvm-commits]

On Fri, Mar 23, 2018 at 7:29 AM, John Brawn via Phabricator <
reviews at reviews.llvm.org> wrote:

> john.brawn added a comment.
>
> In https://reviews.llvm.org/D39960#987169, @spatel wrote:
>
> > I can see the transform that we want to make, but the way that this is
> coded seems wrong to me. We shouldn't have to search the block for existing
> operands to know if the transform is profitable.
> >
> > What we should do instead is start the pattern match from a binop, so
> you're matching selects with a common condition operand and then binops
> with a common operand:
>
>
> That would work for the test cases in the patch, but it's entirely
> possible to have the selects be used in unrelated instructions, e.g.
>
>   define i32 @example(i32 %x, i32 %y, i32* %ptr) {
>     %add1 = add i32 %x, 1
>     %add2 = add i32 %y, 1
>     %cmp = icmp ugt i32 %x, %y
>     %select1 = select i1 %cmp, i32 %x, i32 %y
>     %select2 = select i1 %cmp, i32 %add1, i32 %add2
>     %gep = getelementptr i32, i32* %ptr, i32 1
>     store i32 %select1, i32* %ptr, align 4
>     store i32 %select2, i32* %gep, align 4
>     ret i32 %add1
>   }
>
> Here we want to replace select2 with add i32 %select1, 1 but there's no
> user of select2 which also uses select1.
>

Yes, this is another good example of why using the select form instead of
the phi form often makes analysis and elimination harder.


>
> > Alternatively, couldn't / shouldn't some other pass turn multiple
> selects with the same condition into compare/branch/phi?
>
> As far as I know no (and it would have to be careful to agree with
> FoldTwoEntryPHINode in SimplifyCFG as to when it's a good idea), and I'm
> not sure how exactly it would help here? We'd be looking at phi
> instructions instead of select instructions, but that's it as far as I can
> tell.
>
> We already know how to do a lot of these transforms on phis, and this
transform is a very typical  variant of FRE/PRE (related to the strength
reduction extension).  First, in the above form, nothing can/will detect
the redundancy.  The above is the FRE case. In the similar PRE case
(%select2 = select i1 %cmp, i32 %add1, i32 <something else>), without a
compare/branch/phi there, nothing would understand how to place the
instruction.

IE %select2 = select i1 %cmp, i32 %add1, i32 <something else> ->
add i32 %select1, 1 on the cmp == true path, and actually compute it on the
something else path.


But folks also already know i think adding every single possible variant of
local redundancy elimination to  InstCombine is a bad idea (it can't make
good global decisions, like here, and has to iterate constantly to try to
approximate the global transform badly)
Since we still seem to be doing it just as fast as we can, ...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180323/c2578ab3/attachment.html>