<div dir="ltr"><div>Yuan -</div><div><br></div><div>I've added the following DAG transforms that try to improve codegen for cmp+select patterns:<br></div><div><div>
<div><a href="https://reviews.llvm.org/rL337130" rel="noreferrer" target="_blank">https://reviews.llvm.org/rL337<wbr>130</a></div><div><a href="https://reviews.llvm.org/rL338132" rel="noreferrer" target="_blank">https://reviews.llvm.org/rL338<wbr>132</a></div><div><a href="https://reviews.llvm.org/rL338317" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>rL338317</a><br></div></div><div><br></div><div>Please let me know if that helped your target and/or if there are other patterns that we need to improve.<br></div><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 3, 2018 at 7:25 PM, Yuan Lin <span dir="ltr"><<a href="mailto:yualin@google.com" target="_blank">yualin@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi,<span class=""><div><br></div><div>  "<i>While its true select convertSelectOfCosntantsToMath<wbr>() creates add+sext i1, there's a transform in DAGCombiner::visitADDLike that does "add (sext i1), X -> sub X, (zext i1)". Why didn't that fire for your target?</i>"</div><div><br></div></span><div>  That's a good question! We never explicitly marked sext-on-i1 as a custom op.  And I think the default behavior is "legal" (Am I right on that?)  So the following transform never gets executed because it thinks sext-on-i1 as legal op.</div><div><div><br></div><div> <font face="monospace, monospace"> // add (sext i1), X -> sub X, (zext i1)</font></div></div><div><div><font face="monospace, monospace">  if (N0.getOpcode() == ISD::SIGN_EXTEND &&</font></div><div><font face="monospace, monospace">      N0.getOperand(0).getValueType(<wbr>) == MVT::i1 &&</font></div><div><font face="monospace, monospace">      !TLI.isOperationLegal(ISD::<wbr>SIGN_EXTEND, MVT::i1)) {</font></div><div><font face="monospace, monospace">    SDValue ZExt = DAG.getNode(ISD::ZERO_EXTEND, DL, VT, N0.getOperand(0));</font></div><div><font face="monospace, monospace">    return DAG.getNode(ISD::SUB, DL, VT, N1, ZExt);</font></div><div><font face="monospace, monospace">  }</font></div></div><div><br></div><div>  I will try to change the isel behavior sext-on-i1, and see if that fixes the issue.</div><div><div class="h5"><div><br></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Jul 3, 2018 at 3:47 PM Craig Topper <<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">While its true select <span style="text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">convertSelectOfCosntant<wbr>sToMath() creates add+sext i1, there's a transform in DAGCombiner::visitADDLike that does "add (sext i1), X -> sub X, (zext i1)". Why didn't that fire for your target?</span><div><br clear="all"><div><div dir="ltr" class="m_-7432083076235350611m_-8567260943563699425gmail_signature">~Craig</div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Jul 3, 2018 at 3:44 PM Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>I linked the wrong patch review. Here's the patch that was actually committed:</div><div><a href="https://reviews.llvm.org/D48508" target="_blank">https://reviews.llvm.org/<wbr>D48508</a></div><div class="gmail_extra"><a href="https://reviews.llvm.org/rL335433" target="_blank">https://reviews.llvm.org/<wbr>rL335433</a><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 3, 2018 at 4:39 PM, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>[adding back llvm-dev and cc'ing Craig]<br></div><div><br></div><div>I think you are asking if we are missing a fold (or your target is missing enabling another hook) to transform the sext+add into shift+or? I think the answer is 'yes'. We probably should add that fold. This seems like a similar case as the recent:<a href="https://reviews.llvm.org/D48466" rel="noreferrer" target="_blank"> https://reviews.llvm.org/<wbr>D48466</a><br></div><div><br></div><div>Note that on x86, the sext+add becomes zext+sub:</div><div>        t20: i8 = setcc t3, Constant:i16<-1>, setgt:ch<br>      t24: i16 = zero_extend t20<br>    t17: i16 = sub Constant:i16<5>, t24<br><br></div><div>Would that transform help your target?<br></div></div><div class="m_-7432083076235350611m_-8567260943563699425m_556386071001254764gmail-HOEnZb"><div class="m_-7432083076235350611m_-8567260943563699425m_556386071001254764gmail-h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 3, 2018 at 3:55 PM, Yuan Lin <span dir="ltr"><<a href="mailto:yualin@google.com" target="_blank">yualin@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi, Roman and Sanjay,<div><br></div><div>  Thank you for your reply!  We currently do run DAGCombiner, but didn't implement this specific transformation.  I just tried turning on convertSelectOfCosntantsToMath<wbr>() in our ISelLowering, but that doesn't quite work because it generated a sign_extend op from i1 to i16, which our backend currently doesn't support.  </div><div><br></div><div>  Does the DAGCombiner already has this transformation implemented?</div><div><br></div><div>Thanks,</div><div>--Yuan</div></div><div class="m_-7432083076235350611m_-8567260943563699425m_556386071001254764gmail-m_2227826763313749442HOEnZb"><div class="m_-7432083076235350611m_-8567260943563699425m_556386071001254764gmail-m_2227826763313749442h5"><br><div class="gmail_quote"><div dir="ltr">On Tue, Jul 3, 2018 at 11:22 AM Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Do you run DAGCombiner? And are you overriding TLI.<wbr>convertSelectOfConstantsToMath<wbr>(VT) for your target?</div><div><br></div><div>For the stated example (true val and false val constants in the select differ by 1), that should already be converted.<br></div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 3, 2018 at 12:13 PM, Roman Lebedev <span dir="ltr"><<a href="mailto:lebedev.ri@gmail.com" target="_blank">lebedev.ri@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>On Tue, Jul 3, 2018 at 9:06 PM, Yuan Lin via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
> Hi, Sanjay/all,<br>
><br>
>   I noticed in rL331486 that some compare-select optimizations are disabled<br>
> in favor of providing canonicalized cmp+select to the backend.<br>
><br>
>   I am currently working on a private backend target, and the target has a<br>
> small code size limit.  With this change, some of the apps went over the<br>
> codesize limit.  As an example,<br>
><br>
> C code:<br>
>   b = (a > -1) ? 4 : 5;<br>
><br>
> ll code:<br>
> Before rL331486:<br>
>   %0 = lshr i16 %a.0.a.0., 15<br>
>   %1 = or i16 %0, 4<br>
><br>
> After rL331486:<br>
>   %cmp = icmp sgt i16 %a.0.a.0., -1<br>
>   %cond = select i1 %cmp, i16 4, i16 5<br>
><br>
>   With the various encoding restrictions of my particular target, the<br>
> cmp/select generated slightly larger code size.  However, because the apps<br>
> were very close to the code size limit, this slight change pushed them over<br>
> the limit.<br>
><br>
>   If I still prefer to have this optimization performed, then is my best<br>
> strategy moving forward to implement this optimization as a peephole opt in<br>
> my backend?<br>
</span>I personally think it should probably be a DAGCombine transform,<br>
driven by a Target Transform Info hook (e.g. like hasAndNot()).<br>
<br>
> Thanks,<br>
> --Yuan<br>
Roman.<br>
<br>
> ______________________________<wbr>_________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-dev</a><br>
><br>
</blockquote></div><br></div>
</blockquote></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>
</blockquote></div>
</blockquote></div></div></div></div>
</blockquote></div><br></div>