<div dir="ltr">Hi Ahmed,<br><br>I do agree with you. However unwinding all the way and just using generic IR leaves you quite a long chain of instructions to match which may well be fragile in the face of IR and DAG optimizations. DAG in particular has a really irritating habit of moving nodes like truncates/extends around making long chains like this difficult to match.<div><br></div><div>We've got:</div><div>  sum_all_lanes( abs( sub ) ) to match.</div><div><br></div><div>If sum_all_lanes and abs were single nodes, I think this would be perfectly good enough. As it is, sum_all_lanes is log2(N) shuffles and abs() is a comparison and vector select.</div><div><br></div><div>Perhaps just having intrinsics for horizontal summations and the absolute value might be good enough? I'm not sure.</div><div><br></div><div>Cheers,</div><div><br></div><div>James</div></div><br><div class="gmail_quote">On Thu, 16 Apr 2015 at 18:52 Ahmed Bougacha <<a href="mailto:ahmed.bougacha@gmail.com">ahmed.bougacha@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">FWIW I agree with James' concerns: I thought the intrinsic had already<br>
been agreed upon, but that doesn't seem to be the case.<br>
<br>
This got me thinking, and perhaps that's what you're saying James:  if<br>
the SAD intrinsic is too X86-specific, why do we even need an<br>
intrinsic?   We already express horizontal reductions using the<br>
expanded form (if that's not ideal, then that's a separate topic);<br>
the SUB is just a "sub <16 x i8>".<br>
<br>
The tricky part is the ABS, which AFAIK we express using the expanded<br>
form as well (InstCombine seems to agree).<br>
<br>
If the only reason for the intrinsic is making it easier on the<br>
vectorizer (I haven't really followed that discussion), it sounds like<br>
there's something to be done for the more general problem (e.g. MAX,<br>
or SAT, which I have not gotten back to yet).<br>
<br>
Am I making sense?<br>
<br>
-Ahmed<br>
<br>
On Thu, Apr 16, 2015 at 2:30 AM, James Molloy <<a href="mailto:james.molloy@arm.com" target="_blank">james.molloy@arm.com</a>> wrote:<br>
> Hi,<br>
><br>
> Thanks for continuing to work on this!<br>
><br>
> From a high level, I still stand by the comments I made on your LoopVectorizer review that this is a bit too Intel-specific. ARM and AArch64's SAD instructions do not behave as you model here. You model SAD as a per-element subtract, abs, then a cross-lane (horizontal) summation. The VABD instruction in ARM does not do the last step - that is assumed to be done in a separate instruction.<br>
><br>
> Now, we can model your intrinsic semantics in ARM using:<br>
><br>
>   val2 = ABD val0, val1<br>
>   val3 = ADDV val2<br>
><br>
> However that isn't very efficient - the sum-across-lanes is expensive. Ideally, in a loop, we'd do something like this:<br>
><br>
>   loop:<br>
>     reduction1 = ABD array0[i], array1[i]<br>
>     br loop<br>
>   val = ADDV reduction1<br>
><br>
> So we'd only do the horizontal step once, not on every iteration. That is why I think having the intrinsic represent just the per-element operations, not the horizontal part, would be the lowest common denominator between our two targets. It is easier to pattern match two nodes than to split one node apart and LICM part of it.<br>
><br>
> Similarly, the above construction should the loop vectorizer emit it is not very good for you. You would never be able to match your PSAD instruction. So my suggestion is this:<br>
><br>
> - The intrinsic only models the per-element operations, not the horizontal part.<br>
> - The X86 backend pattern matches the sad intrinsic plus a horizontal reduction to its PSAD node.<br>
> - The Loop Vectorizer has code to emit *either* the per-element-only or the per-element-plus-horizontal SAD as part of its reduction logic.<br>
> - The TargetTransformInfo hook is extended to return an `enum SADType`, in much the same way that `popcnt` type is done currently.<br>
><br>
> How does this sound?<br>
><br>
> Cheers,<br>
><br>
> James<br>
><br>
><br>
> REPOSITORY<br>
>   rL LLVM<br>
><br>
> <a href="http://reviews.llvm.org/D9029" target="_blank">http://reviews.llvm.org/D9029</a><br>
><br>
> EMAIL PREFERENCES<br>
>   <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
><br>
><br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>