[PATCH] D93860: [SLP] delete unused pairwise reduction option

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D93860#2475122 <https://reviews.llvm.org/D93860#2475122>, @pengfei wrote:

> In D93860#2475028 <https://reviews.llvm.org/D93860#2475028>, @spatel wrote:
>
>> In D93860#2473348 <https://reviews.llvm.org/D93860#2473348>, @pengfei wrote:
>>
>>> Hi Sanjay, pairwise reduction seems not adopted by any targets. But do we need to consider the FMF here? I found LangRef says the intrinsic is sequential for fadd and fmul if `reassoc` flag is not set.
>>
>> Hi @pengfei - 
>> Sorry for the delayed reply - I did not get an email notification for your comment here in Phab.
>>
>> We do not need to care about FMF directly here because neither of these shuffle variations corresponds to "sequential" reduction of the elements. In other words, we should require `reassoc` (at the least) to form an fadd or fmul reduction that is expected to need any shuffles. Let me know if you see any holes in that theory.
>>
>> One motivation for this cleanup is to correct bugs with using/propagating FMF (that was also true for D87416 <https://reviews.llvm.org/D87416>). For example - https://llvm.org/PR35538
>
> Thanks for explanation. I was confused it with reduction intrinsics. I noticed `tryToReduce` sets fast flag instead of requires. Is that the problem you mentioned? By the way, can we build reduction intrinsics directly here instead of doing shuffle?

SLP requires `fast` as a pre-condition before matching a reduction (although I'm not sure if that implementation is completely sound), so the hard-coded logic:

  Unsafe.setFast();
  Builder.setFastMathFlags(Unsafe);

...is theoretically safe here. But yes, that is one bug I am hoping to fix. LoopVectorizer appears to have different bug(s).

The code in SLP's emitReduction() is strange - we rely on createSimpleTargetReduction() on one path, but not the other (without this patch).
We should use that utility all the time, so we are not duplicating it here in SLP. I am trying to clean up that code too -  for example 8d18bc8e6db7 <https://reviews.llvm.org/rG8d18bc8e6db717352811a44a81e76a196530f612> .
So that is why I suggested that the alternative to this patch would be to push the shuffle logic into that utility. That way, LoopVectorizer could use it too (if it was actually needed by some target).
Let me know if I am not answering/understanding the questions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93860/new/

https://reviews.llvm.org/D93860