<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Eli,<div class=""><br class=""><div class="">Thanks for pointing to the CTLZ_ZERO_UNDEF “LibCall” implementation. I have not it in the version that I am currently using, so it’s nice to know that it’s implemented now. </div><div class=""><br class=""></div><div class="">Incidentally, the CTLZ… implementation is IDENTICAL to what I am proposing for the Shifts. This is not just adding support for “out-of-tree-targets”, but giving consistency to the fact that we have perfectly defined LibCalls for Shifts, but they can’t be used because there’s an omission in the sources.</div><div class=""><br class=""></div><div class="">I only proposed to add a missing case statement to the ConvertNodeToLibCall.</div><div class=""><br class=""></div><div class="">To my understanding this is totally harmless to any existing or future targets because all what it does is to call the Library function when setOperationAction is set to “LibCall”. This is not different that any other case, and you just confirmed that you added the same for the CTLZ… functions. So I do not understand what you mean by “terrible quality in a lot of cases”: Targets with native shift support are not affected, and neither are targets with custom lowering.</div><div class=""><br class=""></div><div class="">I would suggest that you look in more detail at the actual implementation of the ConvertNodeToLibCall and from where it is called and its actual purpose. There’s nothing else required. So I would appreciate that you elaborate on why you think that this is not a good idea.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class=""><br class=""></div><div class="">John Lluch</div><div class=""><br class=""></div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 10 Jun 2019, at 21:09, Eli Friedman <<a href="mailto:efriedma@quicinc.com" class="">efriedma@quicinc.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="WordSection1" style="page: WordSection1; font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">All in-tree targets have variable shift instructions for native integer types, except for AVR. And AVR implements custom lowering. I’m not sure what else would be required to actually make marking a shift as “libcall” actually work well; technically, the change you’re proposing might produce valid code, but it would be terrible quality in a lot of cases. So I’m not eager to add partial support just for out-of-tree targets.<o:p class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">Marking CTLZ_ZERO_UNDEF as “LibCall” was implemented in<span class="Apple-converted-space"> </span><a href="https://reviews.llvm.org/D47917" style="color: rgb(149, 79, 114); text-decoration: underline;" class="">https://reviews.llvm.org/D47917</a><span class="Apple-converted-space"> </span>. Probably straightforward to extend that to cover CTTZ_ZERO_UNDEF and CTPOP.<o:p class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">-Eli<o:p class=""></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div><div style="border-style: none none none solid; border-left-width: 1.5pt; border-left-color: blue; padding: 0in 0in 0in 4pt;" class=""><div class=""><div style="border-style: solid none none; border-top-width: 1pt; border-top-color: rgb(225, 225, 225); padding: 3pt 0in 0in;" class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><b class="">From:</b><span class="Apple-converted-space"> </span>llvm-dev <<a href="mailto:llvm-dev-bounces@lists.llvm.org" class="">llvm-dev-bounces@lists.llvm.org</a>><span class="Apple-converted-space"> </span><b class="">On Behalf Of<span class="Apple-converted-space"> </span></b>Joan Lluch via llvm-dev<br class=""><b class="">Sent:</b><span class="Apple-converted-space"> </span>Monday, June 10, 2019 8:31 AM<br class=""><b class="">To:</b><span class="Apple-converted-space"> </span>llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a>><br class=""><b class="">Subject:</b><span class="Apple-converted-space"> </span>[EXT] [llvm-dev] Bug: Library functions for ISD::SRA, ISD::SHL, and ISD::SRL<o:p class=""></o:p></div></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">LLVM appears to support Library functions for <span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(79, 129, 135); background-color: white; background-position: initial initial; background-repeat: initial initial;" class="">ISD</span><span style="font-size: 8.5pt; font-family: Monaco, serif; background-color: white; background-position: initial initial; background-repeat: initial initial;" class="">::<span style="color: rgb(49, 89, 93);" class="">SRA </span></span>,<span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(79, 129, 135); background-color: white; background-position: initial initial; background-repeat: initial initial;" class="">ISD</span><span style="font-size: 8.5pt; font-family: Monaco, serif; background-color: white; background-position: initial initial; background-repeat: initial initial;" class="">::<span style="color: rgb(49, 89, 93);" class="">SHL</span></span>, and <span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(79, 129, 135); background-color: white; background-position: initial initial; background-repeat: initial initial;" class="">ISD</span><span style="font-size: 8.5pt; font-family: Monaco, serif; background-color: white; background-position: initial initial; background-repeat: initial initial;" class="">::<span style="color: rgb(49, 89, 93);" class="">SRL</span></span>, as they are properly defined in RuntimeLibCalls.def.<o:p class=""></o:p></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">The library functions defined in RuntimeLibCalls.def (among others) are these:<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; background-color: white;" class=""><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">HANDLE_LIBCALL(SRA_I16, "__ashrhi3")<o:p class=""></o:p></span></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; background-color: white;" class=""><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">HANDLE_LIBCALL(SRA_I32, "__ashrsi3")<o:p class=""></o:p></span></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; background-color: white;" class=""><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">HANDLE_LIBCALL(SRA_I64, "__ashrdi3")<o:p class=""></o:p></span></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">However, setting<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; background-color: white;" class=""><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(49, 89, 93);" class="">setOperationAction</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">(</span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(79, 129, 135);" class="">ISD</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">::</span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(49, 89, 93);" class="">SRA</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">, </span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(79, 129, 135);" class="">MVT</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">::</span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(49, 89, 93);" class="">i16</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">, </span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(49, 89, 93);" class="">LibCall</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">);</span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(49, 89, 93);" class=""><o:p class=""></o:p></span></div></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">in the TargetLowering constructor causes LLVM to stop with an assert as the shift instruction can’t be selected. <o:p class=""></o:p></div></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">The problem is in <span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(79, 129, 135); background-color: white; background-position: initial initial; background-repeat: initial initial;" class="">SelectionDAGLegalize</span><span style="font-size: 8.5pt; font-family: Monaco, serif; background-color: white; background-position: initial initial; background-repeat: initial initial;" class="">::ConvertNodeToLibcall </span>because there’s no switch case for any of the Shift instructions. <o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">The problem gets solved by just adding switch cases like this (and similar for the other shift instructions):<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; background-color: white;" class=""><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(186, 45, 162);" class="">case</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class=""><span class="Apple-converted-space"> </span></span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(79, 129, 135);" class="">ISD</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">::</span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(49, 89, 93);" class="">SRA</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">:</span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(186, 45, 162);" class=""><o:p class=""></o:p></span></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; background-color: white;" class=""><span style="font-size: 8.5pt; font-family: Monaco, serif;" class=""> Results.</span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(49, 89, 93);" class="">push_back</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">(</span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(49, 89, 93);" class="">ExpandIntLibCall</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">(Node,<span class="Apple-converted-space"> </span></span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(186, 45, 162);" class="">false</span><span style="font-size: 8.5pt; font-family: Monaco, serif;" class="">,</span><span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(49, 89, 93);" class=""><o:p class=""></o:p></span></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; background-color: white;" class=""><span style="font-size: 8.5pt; font-family: Monaco, serif;" class=""> <span class="Apple-converted-space"> </span><span style="color: rgb(79, 129, 135);" class="">RTLIB</span>::<span style="color: rgb(49, 89, 93);" class="">SRA_I16</span>,<o:p class=""></o:p></span></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; background-color: white;" class=""><span style="font-size: 8.5pt; font-family: Monaco, serif;" class=""> <span class="Apple-converted-space"> </span><span style="color: rgb(79, 129, 135);" class="">RTLIB</span>::<span style="color: rgb(49, 89, 93);" class="">SRA_I16</span>,<span class="Apple-converted-space"> </span><span style="color: rgb(79, 129, 135);" class="">RTLIB</span>::<span style="color: rgb(49, 89, 93);" class="">SRA_I32</span>,<o:p class=""></o:p></span></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><span style="font-size: 8.5pt; font-family: Monaco, serif; background-color: white; background-position: initial initial; background-repeat: initial initial;" class=""> <span style="color: rgb(79, 129, 135);" class="">RTLIB</span>::<span style="color: rgb(49, 89, 93);" class="">SRA_I64</span>,<span class="Apple-converted-space"> </span><span style="color: rgb(79, 129, 135);" class="">RTLIB</span>::<span style="color: rgb(49, 89, 93);" class="">SRA_I128</span>));</span><o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">I think this is a BUG by omission of necessary switch cases.<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""> <o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">Similarly, the following ISD codes <span style="font-size: 8.5pt; font-family: Monaco, serif; color: rgb(79, 129, 135); background-color: white; background-position: initial initial; background-repeat: initial initial;" class="">ISD</span><span style="font-size: 8.5pt; font-family: Monaco, serif; background-color: white; background-position: initial initial; background-repeat: initial initial;" class="">::<span style="color: rgb(49, 89, 93);" class="">CTTZ, </span><span style="color: rgb(79, 129, 135);" class="">ISD</span>::<span style="color: rgb(49, 89, 93);" class="">CTLZ, </span><span style="color: rgb(79, 129, 135);" class="">ISD</span>::<span style="color: rgb(49, 89, 93);" class="">CTPOP </span></span>do not define any Library calls, despite LLVM being able to fully expand them into rather large code for targets that do not natively implement them.<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">I regard this also as an omission/bug, because not all targets would benefit from the custom expansion of these ISD codes, which as said can get unnecessarily long and costly. <o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">Instead, LLVM should have them available as possible LibCalls.<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">Any comments or opinions on these subjects are appreciated<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif;" class="">Joan Lluch</div></div></div></div></div></blockquote></div><br class=""></div></div></body></html>