<div dir="ltr">Hi Gerolf,<br><br><div class="gmail_quote">On Sun, 26 Apr 2015 at 05:10 Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Please see below.<div><br><div></div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div>On Apr 25, 2015, at 1:44 AM, James Molloy <<a href="mailto:james@jamesmolloy.co.uk" target="_blank">james@jamesmolloy.co.uk</a>> wrote:</div><br><div><div dir="ltr">Hi,<br><br>I've also previously gone down the lines of a target-isel approach and had a patch that pretty much worked. I'll explain why I now think this is better done early in the IR. I should say though that I could be convinced to go back to the target-isel approach if there are better reasons :)</div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><div>I’m more curious to hear about your arguments and in particular what performance headroom you foresee. One of our CPU architects had pointed out that we could generate smax/umax etc., which took me longer than it should have. So I thought about doing it at a higher level also, looked around, looked again, scratched my head and relatively quickly talked myself into something  like 'lot of work, little headroom, target level support good enough' :-)</div></div></div></div></blockquote><div><br></div><div>The performance improvements I see and the rationale for this is where you have multiple mins/maxs in series. Several benchmarks I have such as image filters use min/max to implement clamp(); obviously image filters tend to be very small so every extra instruction counts. Similarly another image filter uses a ternary min (min(a,b,c)), which ends up as min/min. That currently doesn't get matched either.</div><div><br></div><div>I can see min/max reductions as being easier to identify (and either vectorize or reassociate, both having big speedup potential). Finally and perhaps the biggest headroom is in saturation. Matching the idiom clamp(x, 0, 255), you can generate a VQMOV. Even better, if the datatype is then shrunk as it often is after such a clamp, you end up with one VQMOVN. That's 5 instructions -> 1 instruction.</div><div><br></div><div>Obviously the effect this would have depends on the domain - image filters (such as sharpen from Geekbench) are where this is really going to give dividends. But even for non-image-filters, there's still a hygiene factor where we really should be generating the right thing even if we don't get a massive performance boost.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>1. Matching min/max patterns can be awkward in the presence of constants. Consider the idiomatic max(x, 0) with a conversion after it:</div><div>  </div><div>  %1 = fcmp ogt float 0.0, float %x</div><div>  %2 = select i1 %1, float %x, float 0.0</div><div>  %3 = fptoui float %2 to i32</div><div><br></div><div>InstCombine will push the fptoui up:</div><div><br></div><div>  <span style="font-size:13.1999998092651px;line-height:19.7999992370605px">%1 = fcmp ogt float 0.0, float %x</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">  %2 = fptoui float %x to i32</span><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div style="font-size:13.1999998092651px;line-height:19.7999992370605px">  %3 = select i1 %1, i32 %2, i32 0</div><div style="font-size:13.1999998092651px;line-height:19.7999992370605px">  </div><div>Now, you have a much more complex pattern to match. You have to identify that "i32 0" is the same as "float 0.0" with "fptoui" applied to it. This gets even more complex when you have nested mins and maxes, for example in "clamp(x, 0, 255)". It was this that I was having difficulty efficiently and reliably matching at the ISel level, and what convinced me to move higher up the pipe where at least testing was easier.</div><div><br></div><div>Now actually, we can do even better in IR and move further still up the pipe, and run before InstCombine. Then, we don't need any complex matching at all and everything is simpler.</div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div><div>I think of this as short-coming of inst combine. It could convert the float to int before the cmp resulting in int cmp/select. Since this an example for fp -> int conversion, here is another thought:  It seems that this fmin/max -> imin/max conversion is always the right thing to do, and the IR level is appropriate, but in some cases that depends on the HW. For example, if there are two instances of that pattern one might want to convert one to fp and one to int so both can execute in parallel (assuming the HW provides the proper resources). </div></div></div></div></blockquote><div><br></div><div>Yes, I think that's right, and I was going to control this with a target hook. I'm seriously considering whether my pass might not be better written as an instcombine rather than a standalone pass... haven't come to any conclusions yet.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div><br></div><div>The issue with complex pattern recognition at the ISEL level: does this point to a general weakness that also effect over pattern, or is this an isolated instance min/max at IR level will take care of?</div></div></div></div><div style="word-wrap:break-word"><div><div><br></div></div></div></blockquote><div>I think it's generally that while there are one or two patterns you'd need to match at IR level, because there are more, specialized SDAG nodes the problem becomes larger at that point. Also, it just stands to reason that if you have a hard-to-match sequence, the longer you keep it around unmatched the higher the likelihood of it being destroyed when you want to match it.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>2. As I've mentioned, this approach allows anything using the cost model to be more accurate. This currently is just the vectorizers but I see other users coming along soon like loop distribution and loop fusion.</div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div>Would be good if you could share examples where the difference matters. I expect we’ll have to look into the cost model much more closely soon.</div></div></div><div style="word-wrap:break-word"><div><div><br></div></div></div></blockquote><div>I don't have explicit examples right now. I'm more thinking of hygiene factor - making the model more accurate is a good thing regardless of immediate benefit. But it should also help the register pressure estimation code get a better result. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>3. This simplifies pattern matching in backends with not just min/max support, but also with explicit "clamp" support such as GPUs. GPUs will often have (almost free) clamp_0_255 instructions or modifiers as this is used extremely often in image filters and shaders.</div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div>Ok, I take your word for it.</div></div></div><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>4. Doing this at the IR level is immediately useful for all backends, not just (one of) AArch64 or AArch32.</div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div>Ok, although I expect all targets for which it matters have that support.</div></div></div><div style="word-wrap:break-word"><div><div><br></div></div></div></blockquote><div>Right, although demonstrably it doesn't work at the moment, at least not for AArch64 and AArch32. My experiments have shown that X86 isn't fantastic either. Isolated mins/maxes, yes. Coupled mins/maxes? no. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>5. To make sure the clamp() idiom got matched, I upped the amount that SimplifyCFG would speculate (in r229099). This caused some slight controversy - it is still in-tree but there is still the potential of pulling it out if it is found that it was too aggressive. The IR approach means we don't actually need it and can pattern match across basic blocks, which we can't do in ISel (and it'd be too late by then because all the vectorizers will have bailed out).</div></div></div></blockquote><div><br></div></div></div></div><div style="word-wrap:break-word"><div><div>That is the strongest argument I see so far. There is a dependence between optimizations. And the example will come where there is just not enough speculation in SimplifyCFG for the next pattern to get recognized.</div></div></div></blockquote><div>Right. min+min+min (horizontal reduce) won't currently get speculated. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>As you've said Gerolf, we have patchy max/min/abs matching and handling in many places in codegen; it would be nice if this approach could remove them all. My only worry is that at the moment, if a target does not support the new intrinsics they will be expanded, but will be expanded only after the DAG is type-legalized and so may miss out on some scalar optimizations. I'm not sure how well-founded this worry is though.</div></div></div></blockquote></div></div></div><div style="word-wrap:break-word"><div><div>Surprises will happen anyway with a (re-) design like that. Just the scraps to deal with when they happen.</div><div><br></div><div>Would the new design make most the HW specific int/fp/vector intrinsics in ./inc </div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>lude/llvm/IR redundant?</div></div></div><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div><br></div></div></div></blockquote></div></div></div></blockquote><div>I certainly expect the new intrinsics to map directly to the smin/umin NEON intrinsics in the A32 and A64 backends. It'd be nice if that was true for other architectures too but I just don't know. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div><div dir="ltr"><div></div><div>Cheers,</div><div><br></div><div>James</div><div><br></div></div><br><div class="gmail_quote">On Sat, 25 Apr 2015 at 04:19 Owen Anderson <<a href="mailto:resistor@mac.com" target="_blank">resistor@mac.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Apr 24, 2015, at 2:19 PM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com" target="_blank">ghoflehner@apple.com</a>> wrote:</div><br><div><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">I was wondering if there were a performance advantage to have a higher level representation of min/max. My hunch is that recognizing the idiom at the low-level is ‘good enough’.</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"></div></blockquote></div><br></div><div style="word-wrap:break-word"><div>I suspect (aside from vectorization) the impact to be minimal for integer min/max.  For floating point, I expect there to be greater upside because of ISA level support, and because the SW emulation expansion is typically complex.</div></div><div style="word-wrap:break-word"><div><br></div><div>—Owen</div></div></blockquote></div>
</div></blockquote></div></div></div></blockquote></div></div>