[PATCH] D54719: [Intrinsic] Signed Fixed Point Multiplication Intrinsic

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan added inline comments.


================
Comment at: llvm/docs/LangRef.rst:12830
+value is rounded up or down to the closest representable value. The rounding
+direction is unspecified.
+
----------------
leonardchan wrote:
> leonardchan wrote:
> > efriedma wrote:
> > > leonardchan wrote:
> > > > bjope wrote:
> > > > > ebevhan wrote:
> > > > > > efriedma wrote:
> > > > > > > "The rounding direction is unspecified" seems scary to me... is there really no standard for rounding these operations?
> > > > > > The Embedded-C standard says that the rounding direction is implementation-defined, and is allowed to be indeterminable. 
> > > > > > 
> > > > > > I also think it's a bit unfortunate that we can't express the rounding direction, since it inhibits optimization, but locking it down to either rounding up or down will make things annoying for any target with operations that round the other direction.
> > > > > > 
> > > > > > Saying that it's unspecified lets us properly legalize this to something sensible while allowing targets to implement it with legal operations if they have them.
> > > > > > 
> > > > > > For what it's worth, the legalization will make it round down (toward negative infinity).
> > > > > Maybe it should say "target specific" instead? And then the legalization (and any constant folding etc) could use some target transform hook to decide in which direction to do the rounding (I guess it could be set to up/down/any).
> > > > > 
> > > > > Does "unspecified" mean that ConstantFolding use one strategy for rounding and legalization another strategy? Or should we only allow constant folding when rounding isn't a problem (such as multiply by zero)?
> > > > > 
> > > > > Things could still be annoying for a target that does care about rounding. Or maybe it isn't allowed for a target to decide what the implementation-defined behavior should be? Or should it be the same for all targets?
> > > > > 
> > > > > A first implementation could of course go for only supporting "unspecified" (if it is clear what kind of constant folding and other optimizations that are allowed). If a target needs a "target specific" behavior, then I assume the code could be sprinkled with hooks at a later stage.
> > > > > 
> > > > > Maybe it should say "target specific" instead? And then the legalization (and any constant folding etc) could use some target transform hook to decide in which direction to do the rounding (I guess it could be set to up/down/any).
> > > > 
> > > > I would be ok with adding this later. For now I was focusing on laying down the basic code necessary for just using this intrinsic.
> > > > 
> > > > > Does "unspecified" mean that ConstantFolding use one strategy for rounding and legalization another strategy? Or should we only allow constant folding when rounding isn't a problem (such as multiply by zero)?
> > > > 
> > > > I initially had it as "target specific" but changed it to "unspecified" since the rest of the LangRef doc seems to use this term for describing either implementation-defined, or undefined, behavior.
> > > > 
> > > > > Things could still be annoying for a target that does care about rounding. Or maybe it isn't allowed for a target to decide what the implementation-defined behavior should be? Or should it be the same for all targets?
> > > > > A first implementation could of course go for only supporting "unspecified" (if it is clear what kind of constant folding and other optimizations that are allowed). If a target needs a "target specific" behavior, then I assume the code could be sprinkled with hooks at a later stage.
> > > > 
> > > > The hooks are a good idea. I figure for optimizations, depending on what's supported (up/down/any), InstCombine and others can use the hook to determine rounding direction.
> > > Would it make sense to make the rounding mode a parameter to the intrinsic?  That way, frontends can choose the semantics they want, and the semantics are clear throughout the optimization pipeline.  clang could choose the rounding mode in a target-specific manner if necessary.
> > The only thing that I think would be of concern is just having too many parameters, or having an extra intrinsic to dictate rounding direction. Other than this, I have no other strong opinions regarding this.
> > 
> > I imagine though that having an extra parameter/intrinsic to specify rounding would imply that a particular target has the option of specifying rounding in both directions, when really a target would likely just have native rounding in one direction (although this is just an assumption).
> @ebevhan @bjope Do you think an extra parameter/intrinsic is worth adding also in this patch?
It's definitely handy to be able to express the rounding of the operations in the intrinsic, but I think some of the downsides are:
* The number of parameters grows, making the intrinsic more unwieldy.
* There are other roundings than just up and down; rounding towards or away from zero (the former of which I think the fixed-point division ought to try and always follow) are also possibilities.
* We need to support legalization of every rounding mode we add to the intrinsic. Then again, if we add a hook we need to do the same anyway.
* The legalization checks need to check both rounding and scale.

And as you say, it's more likely that the rounding of a specific target should be the same across the board rather than be configurable per operation, if it has support for fixed-point.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54719