[PATCH] D55720: [Intrinsic] Signed Fixed Point Saturation Multiplication Intrinsic

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: llvm/docs/LangRef.rst:13355
+value is rounded up or down to the closest representable value. The rounding
+direction is unspecified.
+
----------------
bjope wrote:
> leonardchan wrote:
> > bjope wrote:
> > > I think this part about unspecified rounding direction either need to be explained more thoroughly somewhere, or we need to find a way to make it possible to specify it. Since the same problem already exist for smul.fix and umul.fix this shouldn't neccessarily be a stopper for this patch,. But I think it poses some problems that there sometimes are two correct results.
> > > 
> > > Are we for example supposed to prohibit constant folding (allowing backend targets implement a specific rounding scheme)?
> > > Then I guess we can't implement the promotion/legalization part either, at least not without specifying which rounding scheme the legalization will use. It would be weird to prohibit constant folding in the first place, while at the same time legalization into generic ISD operations might result in DAG combiner actually ending up constant folding the expression.
> > Hmm. I'm starting to regret not specifying an argument for rounding when making these intrinsics. Would there be any large consequences for changing the intrinsics to accept a 4th argument for rounding?
> Had a short discussion with @ebevhan about this (offline).
> 
> Adding the 4th argument for rounding would
> - make things more clear (avoiding "unspecified")
> - make things more complicated (how many rounding modes should be supported? do we need to support folding/promotion/lowering etc for all different kinds of rounding modes? how do we verify all those modes?)
> 
> So despite my comment above, we think that the way forward is to keep the solution with "unspecified" for now (we already got it for the non-saturating intrinsics).
> 
> But to avoid confusion when people are reading the LangRef and looking at the code etc. we probably want to describe what "rounding direction is unspecified" means somewhere (for example in the introduction about "Fixed Point Arithmetic Intrinsics"). Explaining things like:
> - different optimizations (and legalization) in the pipeline are free to do the rounding in whatever direction they want to (but I think the accuracy of the result still should be well-defined so we need to say something about that?)
> - KnownBits/ValueTracking can't assume the direction of the rounding.
> 
I added more detail to the overview explaining the default expansion for multiplication and rounding to say that targets should specify their own hooks if they care about rounding and optimizations/legalizations should be performed based off that hook. Let me know if there's something else important that should be added.


================
Comment at: llvm/docs/LangRef.rst:13305
 
+Fixed Point Saturation Arithmetic Intrinsics
+---------------------------------
----------------
bjope wrote:
> Do we need a new "chapter" for this?
> 
> Maybe we can just continue the "Fixed Point Arithmetic Intrinsics" chapter here, and skip the general description below that only refers to "Fixed Point Arithmetic Intrinsics" and "Saturation Arithmetic".
> 
> If needed, we can add the "(see Saturation Arithmetic)" somewhere in the semantic description of llvm.smul.fix.sat.* instead.
We probably don't need this. Especially since the fixed point section comes after the saturation section.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2713
 
 void DAGTypeLegalizer::ExpandIntRes_MULFIX(SDNode *N, SDValue &Lo,
                                            SDValue &Hi) {
----------------
bjope wrote:
> Even if rounding is unspecified, I believe this code is implementing some kind of rounding scheme. Should we perhaps say something about this in the function header. It can be at help when looking at the code in the future. Both to understand what the intention was with the original algorithm. And to understand the expected result when looking at test results etc. Or for some target to understand why a "legal"/"custom" lowering gives different result compared to "expand".
Added


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55720