[PATCH] D30086: Add generic IR vector reductions

[Amara Emerson via Phabricator via llvm-commits]

aemerson marked 5 inline comments as done.
aemerson added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:1191
+
+Value *llvm::createTargetReduction(IRBuilder<> &Builder,
+                                   const TargetTransformInfo *TTI,
----------------
rengolin wrote:
> aemerson wrote:
> > delena wrote:
> > > You implemented 2 functions with the same name. It's not clear what the difference between them. Please add some comments before each of them.
> > I've added some comments to the API in the header, but I'll flesh those out more and add some short ones here too.
> I think they're different enough that they deserve different names. Normally, you should use the same name if the arguments are different, but the functionality is the same. Here, the implementation and the logic are wildly different that naming them the same thing could be misleading.
> 
> I haven't looked deep to know if there is SLP logic here that could have remained in the SLP vectorizer's code, or if you could separate the implementation where one is just a wrapper to the other. This would make it more clear, and probably easier to maintain.
I'll rename the functions but I'd argue that the two functions are intended to have the same general functionality, creating vector reductions. The difference is that one of them requires a recurrence descriptor which some clients may not have (SLP vs LV), and the other is a simpler API at the cost of not being able to generate min/max reductions.

The SLP's pairwise reduction code is different to the normal reduction shuffle IR sequence which is why I left that there, there aren't any other users of that code to justify re-factoring yet.


Repository:
  rL LLVM

https://reviews.llvm.org/D30086