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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope 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.
+
----------------
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.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4448
                   isOperationLegalOrCustom(ISD::ADDE, VT));
+
   if (UseGlue)
----------------
Is this newline by mistake? Seems unrelated to the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55720