[PATCH] D30086: Add generic IR vector reductions

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:1191
+
+Value *llvm::createTargetReduction(IRBuilder<> &Builder,
+                                   const TargetTransformInfo *TTI,
----------------
rengolin wrote:
> aemerson wrote:
> > rengolin wrote:
> > > aemerson wrote:
> > > > 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.
> > > That's why I said this smells like SLP/LV are leaking code here... :)
> > It's not leaking, part of the purpose of this patch is to factor out common code from clients emitting identical reduction code.
> Well, it seems that the LV calls one and the SLP calls another, and they're anything but common.
> 
> True, their return value is the same and they have the same purpose, but they're hardly the same thing.
> 
> This means any change/refactoring in one will require similar to the other, and this will not always be clear.
By common code I meant the actual mechanism of generating the shuffle pattern if the target doesn't request to use the intrinsic forms. That code is essentially duplicated between the SLP and LV. Anyway, I'll continue with the requested changes.


Repository:
  rL LLVM

https://reviews.llvm.org/D30086