<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 23, 2018 at 7:29 AM, John Brawn via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">john.brawn added a comment.<br>
<span class=""><br>
In <a href="https://reviews.llvm.org/D39960#987169" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D39960#987169</a>, @spatel wrote:<br>
<br>
> 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.<br>
><br>
> 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:<br>
<br>
<br>
</span>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.<br>
<br>
  define i32 @example(i32 %x, i32 %y, i32* %ptr) {<br>
    %add1 = add i32 %x, 1<br>
    %add2 = add i32 %y, 1<br>
    %cmp = icmp ugt i32 %x, %y<br>
    %select1 = select i1 %cmp, i32 %x, i32 %y<br>
    %select2 = select i1 %cmp, i32 %add1, i32 %add2<br>
    %gep = getelementptr i32, i32* %ptr, i32 1<br>
    store i32 %select1, i32* %ptr, align 4<br>
    store i32 %select2, i32* %gep, align 4<br>
    ret i32 %add1<br>
  }<br>
<br>
Here we want to replace select2 with add i32 %select1, 1 but there's no user of select2 which also uses select1.<br></blockquote><div><br></div><div>Yes, this is another good example of why using the select form instead of the phi form often makes analysis and elimination harder.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Alternatively, couldn't / shouldn't some other pass turn multiple selects with the same condition into compare/branch/phi?<br>
<br>
</span>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.<br>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div>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.</div><div><br></div><div>IE <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">%select2 = select i1 %cmp, i32 %add1, i32 <something else> -> </span></div><div>add i32 %select1, 1 on the cmp == true path, and actually compute it on the something else path.</div><div><br></div><div><br></div><div>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)</div><div>Since we still seem to be doing it just as fast as we can, ...</div><div><br></div></div><br></div></div>