[PATCH] D105432: [Analysis] Add simple cost model for strict (in-order) reductions

[David Sherwood via Phabricator via llvm-commits]

david-arm marked an inline comment as done.
david-arm added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1170
 
+  enum class ReductionType : uint8_t { Split, Ordered };
+
----------------
dmgreen wrote:
> david-arm wrote:
> > dmgreen wrote:
> > > sdesmalen wrote:
> > > > RKSimon wrote:
> > > > > Please can you add doxygen description comments for these?
> > > > nit: may I request this (and any other references to it) to be renamed to TreeWise? In my experience that's a more common term used with these kinds of reductions.
> > > Could we represent this by passing FastMathFlags, not a new enum? The fast math flags are what dictates in-order vs relaxed for example. And I don't believe that an integer "Ordered" reduction is ever different from a un-ordered reduction.
> > I guess we could do that, but the flags contain a lot more than just the reassoc flag, which is the only thing we care about here. I personally think it seems a bit more obvious to the caller what's going on if we specify the type directly. Not sure what others think?
> That would be my point really, that it contains more info. Why invent a new enum when the fast math flags already represent all the information we need here. It is the fundamental information we require here, so why not use it directly?
> 
> We are trying to get the cost of a reduction intrinsic, and that intrinsic has some FMFlags. Those flags may dictate the cost of the resulting code, so the FMF's should be passed to the cost function.
> 
> fmin/fmax could be in the same situations. It could be possible (although I'm not sure it occurs anywhere at the moment) that a "no-nan" fmax reduction has a different cost to one without the flag.
Would there ever be a case where we didn't have the fast math flags, but wanted to calculate a theoretical reduction cost anyway? Then you'd have to explicitly create a FastMathFlags object and have to know exactly which flag needs setting in order to get the desired cost? For example, the user would have to know to set the reassoc flag in order to get the tree-based reduction cost. I mean, not saying that's a bad thing necessarily, but it felt a bit less intuitive and would certainly require additional comments above the `getArithmeticReductionCost` function saying how the flags map precisely to the cost.

Would there potentially be value in having an overloaded interface that maps one to the other (FastMathFlags<>Enum), or do you strongly prefer a single interface that takes FastMathFlags?

I wonder if other reviewers have a preference here?


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

https://reviews.llvm.org/D105432