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

Tue May 14 12:52:12 PDT 2019

leonardchan marked 4 inline comments as done.
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:
> 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?

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55720