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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan marked an inline comment as done.
leonardchan 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.
+
----------------
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).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54719