[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:
> > > > > > 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.
> If the Add/Fadd cases are already covered by this function, and they end up calling the same methods to create the reduction nodes, and half of the arguments are the same, and the purpose is the same and the return value is the sane, I still don't understand why the function above can't be a wrapper to this one.
> 
> Creating a recurrence descriptor doesn't seem that hard, and you don't need to duplicate the Add/Fadd logic up there.
My reasoning is that any improvement in this piece of code due to your suggestion will be superficial at best, and IMO is outweighed by a conceptual cost. .

A recurrence descriptor holds information about a particular recurrence operation, and there's fairly complex code dedicated to using SCEV and LoopInfo to do all the analysis and create one in `RecurrenceDescriptor::isReductionPHI()` which also calls `RecurrenceDescriptor::AddReductionVar()`. The resulting object at a conceptual level models a //recurrence//, which by definition has to be in a loop. Creating a recurrence descriptor in the `createSimpleTargetReduction()` function means creating a "fake" recurrence descriptor, one which may not actually model a recurrence.

Ultimately the createSimpleTargetReduction() is not a wrapper because they cover different functionality at a higher level.


Repository:
  rL LLVM

https://reviews.llvm.org/D30086